Re: [Xen-devel] [PATCH v3 08/10] vpci: register as an internal ioreq server

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2019 15:32, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -29,6 +29,7 @@
>  
>  #include <asm/bzimage.h>
>  #include <asm/dom0_build.h>
> +#include <asm/hvm/ioreq.h>
>  #include <asm/hvm/support.h>
>  #include <asm/io_apic.h>
>  #include <asm/p2m.h>

This is the only change to this file, and there's no addition to
asm/hvm/ioreq.h - how come this #include is needed?

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -20,6 +20,8 @@
>  #include <xen/sched.h>
>  #include <xen/vpci.h>
>  
> +#include <asm/hvm/ioreq.h>

This of course means a step away from the code here being generic
enough such that Arm could eventually use it. Independent of this
aspect perhaps the #include would better move ...

> @@ -478,6 +480,61 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, 
> unsigned int size,
>      spin_unlock(&pdev->vpci->lock);
>  }
>  
> +#ifdef __XEN__

... here?

> +static int ioreq_handler(ioreq_t *req, void *data)
> +{
> +    pci_sbdf_t sbdf;
> +
> +    /*
> +     * NB: certain requests of type different than PCI are broadcasted to all
> +     * registered ioreq servers, ignored those.
> +     */
> +    if ( req->type != IOREQ_TYPE_PCI_CONFIG || req->data_is_ptr )
> +        return X86EMUL_UNHANDLEABLE;

I understand the left side of the || , but why the right side? There
shouldn't be any IOREQ_TYPE_PCI_CONFIG requests with data_is_ptr set,
should there? I also didn't think requests with data_is_ptr set would
get broadcast?

> +int vpci_register_ioreq(struct domain *d)
> +{
> +    ioservid_t id;
> +    int rc;
> +
> +    if ( !has_vpci(d) )
> +        return 0;
> +
> +    rc = hvm_create_ioreq_server(d, HVM_IOREQSRV_BUFIOREQ_OFF, &id, true);
> +    if ( rc )
> +        return rc;
> +
> +    rc = hvm_set_ioreq_handler(d, id, ioreq_handler, NULL);
> +    if ( rc )
> +        return rc;
> +
> +    if ( is_hardware_domain(d) )
> +    {
> +        /* Handle all devices in vpci. */
> +        rc = hvm_map_io_range_to_ioreq_server(d, id, XEN_DMOP_IO_RANGE_PCI,
> +                                              0, ~(uint64_t)0);
> +        if ( rc )
> +            return rc;
> +    }
> +
> +    rc = hvm_set_ioreq_server_state(d, id, true);
> +    if ( rc )
> +        return rc;
> +
> +    return rc;

This last sequence of statements looks a little odd - is this in
anticipation of further additions to the function?

Furthermore the only caller expects the function to clean up after
itself (i.e. partially completed setup be undone), which doesn't
look to be the case here. I can't seem to be able to convince
myself that all of this gets cleaned up.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel