Re: [Xen-devel] [PATCH v2 20/30] xen/x86: add the basic infrastructure to import QEMU passthrough code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.09.16 at 17:57, <roger.pau@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -632,6 +632,8 @@ int hvm_domain_initialise(struct domain *d)
>              goto fail1;
>          }
>          memset(d->arch.hvm_domain.io_bitmap, ~0, HVM_IOBITMAP_SIZE);
> +        INIT_LIST_HEAD(&d->arch.hvm_domain.pt_devices);

This field appears to be redundant with arch.pdev_list.

> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -46,6 +46,10 @@
>  #include <xen/iocap.h>
>  #include <public/hvm/ioreq.h>
>  
> +/* Set permissive mode for HVM Dom0 PCI pass-through by default */
> +static bool_t opt_dom0permissive = 1;

Plain bool / true / false please. And as mentioned by Andrew, we
should stop adding more dom0xyz options, and use a consolidated
dom0= one instead.

> @@ -258,12 +262,403 @@ static bool_t hw_dpci_portio_accept(const struct 
> hvm_io_handler *handler,
>      return 0;
>  }
>  
> +static struct hvm_pt_device *hw_dpci_get_device(struct domain *d)
> +{
> +    uint8_t bus, slot, func;
> +    uint32_t addr;
> +    struct hvm_pt_device *dev;
> +
> +    /* Decode bus, slot and func. */
> +    addr = CF8_BDF(d->arch.pci_cf8);
> +    bus = PCI_BUS(addr);
> +    slot = PCI_SLOT(addr);
> +    func = PCI_FUNC(addr);
> +
> +    list_for_each_entry( dev, &d->arch.hvm_domain.pt_devices, entries )
> +    {
> +        if ( dev->pdev->seg != 0 || dev->pdev->bus != bus ||

Okay, there's no way segments other than 0 can be handled here.
But having glanced over the titles of the rest of the series - where
are those going to be handled (read: Where is the MCFG code,
which qemu doesn't have)?

Also I think the function name is not well chosen: Its prefix suggests
some kind of "official" interface, yet it really just is an internal helper
which doesn't even "get" a device in the general sense of needing to
"put" it later on.

And it looks like the parameter could be constified (but this appears
to be a wider problem).

> +/* Dispatchers */
> +
> +/* Find emulate register group entry */
> +struct hvm_pt_reg_group *hvm_pt_find_reg_grp(struct hvm_pt_device *d,
> +                                             uint32_t address)

Please don't needlessly use fixed width types.

> +{
> +    struct hvm_pt_reg_group *entry = NULL;
> +
> +    /* Find register group entry */
> +    list_for_each_entry( entry, &d->register_groups, entries )
> +    {
> +        /* check address */
> +        if ( (entry->base_offset <= address)
> +             && ((entry->base_offset + entry->size) > address) )

Coding style (&& belongs on the previous line).

And actually I guess I'll stop here, realizing that I'm completely
unconvinced of the not even spelled out intentions. Alone the
lifting of code from qemu is problematic imo: That code has proven
to have many issues, only the most severe of which have got fixed
over time. I'm therefore of the opinion that a clean re-write from
scratch should at least be considered, once it was written down
somewhere (docs/misc/hvmlite.markdown?) and agreed upon what
the behavior actually ought to be.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel