Re: [PATCH] fuzz: map all BARs and enable PCI devices

From: Alexander Bulekov <alxndr@bu.edu>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: Laurent Vivier <lvivier@redhat.com>,
	Thomas Huth <thuth@redhat.com>,
	qemu-devel@nongnu.org, Darren Kenny <darren.kenny@oracle.com>,
	Bandan Das <bsd@redhat.com>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH] fuzz: map all BARs and enable PCI devices
Date: Thu, 10 Dec 2020 08:54:59 -0500	[thread overview]
Message-ID: <20201210135459.w6o7chqwmrorrrmz@mozz.bu.edu> (raw)
In-Reply-To: <114d98b5-5c4e-6315-d91d-92c6baf49d09@redhat.com>

On 201210 1411, Philippe Mathieu-Daudé wrote:
> On 12/10/20 12:36 PM, Darren Kenny wrote:
> > Hi Alex,
> > 
> > On Wednesday, 2020-12-09 at 15:10:54 -05, Alexander Bulekov wrote:
> >> Prior to this patch, the fuzzer found inputs to map PCI device BARs and
> >> enable the device. While it is nice that the fuzzer can do this, it
> >> added significant overhead, since the fuzzer needs to map all the
> >> BARs (regenerating the memory topology), at the start of each input.
> >> With this patch, we do this once, before fuzzing, mitigating some of
> >> this overhead.
> >>
> >> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> > 
> > In general this looks good, I've a small comment/nit below, but nothing
> > serious, so:
> > 
> > Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> > 
> >> ---
> >>  tests/qtest/fuzz/generic_fuzz.c | 23 +++++++++++++++++++++++
> >>  1 file changed, 23 insertions(+)
> >>
> >> diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
> >> index 07ad690683..d95093ee53 100644
> >> --- a/tests/qtest/fuzz/generic_fuzz.c
> >> +++ b/tests/qtest/fuzz/generic_fuzz.c
> >> @@ -16,6 +16,7 @@
> >>  
> >>  #include "hw/core/cpu.h"
> >>  #include "tests/qtest/libqos/libqtest.h"
> >> +#include "tests/qtest/libqos/pci-pc.h"
> >>  #include "fuzz.h"
> >>  #include "fork_fuzz.h"
> >>  #include "exec/address-spaces.h"
> >> @@ -762,6 +763,22 @@ static int locate_fuzz_objects(Object *child, void *opaque)
> >>      return 0;
> >>  }
> >>  
> >> +
> >> +static void pci_enum(gpointer pcidev, gpointer bus)
> >> +{
> >> +    PCIDevice *dev = pcidev;
> >> +    QPCIDevice *qdev;
> >> +
> >> +    qdev = qpci_device_find(bus, dev->devfn);
> >> +    g_assert(qdev != NULL);
> >> +    for (int i = 0; i < 6; i++) {
> >> +        if (dev->io_regions[i].size) {
> >> +            qpci_iomap(qdev, i, NULL);
> >> +        }
> >> +    }
> >> +    qpci_device_enable(qdev);
> >> +}
> >> +
> >>  static void generic_pre_fuzz(QTestState *s)
> >>  {
> >>      GHashTableIter iter;
> >> @@ -810,6 +827,12 @@ static void generic_pre_fuzz(QTestState *s)
> >>          exit(1);
> >>      }
> >>  
> >> +    QPCIBus *pcibus;
> > 
> > NIT: I'm not a huge fan of defining variables in the middle of code,
> >      call me old-fashioned if you will, but I tend to prefer them at the
> >      top of the function, or block ;)
> 
> This is barely tolerated in for(;;) loops.
> 
> See commit 7be41675f7c ("configure: Force the C standard to gnu99")
> and QEMU CODING_STYLE.rst:
> 
>  Declarations
>  ============
> 
>  Mixed declarations (interleaving statements and declarations within
>  blocks) are generally not allowed; declarations should be at the
>  beginning of blocks.
> 
>  Every now and then, an exception is made for declarations inside a
>  #ifdef or #ifndef block: if the code looks nicer, such declarations can
>  be placed at the top of the block even if there are statements above.
>  On the other hand, however, it's often best to move that #ifdef/#ifndef
>  block to a separate function altogether.
> 
> Regards,
>

Sounds good - I'll send out a v2.
Thanks
-Alex

> Phil.
>