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

From: Darren Kenny <darren.kenny@oracle.com>
To: Alexander Bulekov <alxndr@bu.edu>, qemu-devel@nongnu.org
Cc: Laurent Vivier <lvivier@redhat.com>,
	Thomas Huth <thuth@redhat.com>, Alexander Bulekov <alxndr@bu.edu>,
	Bandan Das <bsd@redhat.com>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	philmd@redhat.com
Subject: Re: [PATCH] fuzz: map all BARs and enable PCI devices
Date: Thu, 10 Dec 2020 11:36:57 +0000	[thread overview]
Message-ID: <m2im99ao4m.fsf@oracle.com> (raw)
In-Reply-To: <20201209201054.391408-1-alxndr@bu.edu>

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 ;)

     It does look good in the diff, but would seem odd in the overall
     code.

Thanks,

Darren.