Re: [Qemu-devel] [PATCH v3 2/8] hcd-xhci: check & correct param before using it

[Markus Armbruster]

Cao jin <address@hidden> writes:

> Param checking/correcting code of xchi->numintrs should be placed before
> it is used.
> Also move some resource-alloc code down, save the strenth to free them
> on msi_init's failure.
>
> CC: Gerd Hoffmann <address@hidden>
> CC: Markus Armbruster <address@hidden>
> CC: Marcel Apfelbaum <address@hidden>
> CC: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Cao jin <address@hidden>
> ---
>  hw/usb/hcd-xhci.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 188f954..95b1954 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -3594,25 +3594,6 @@ static void usb_xhci_realize(struct PCIDevice *dev, 
> Error **errp)
>      dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
>      dev->config[0x60] = 0x30; /* release number */
>  
> -    usb_xhci_init(xhci);
> -
> -    if (xhci->msi != ON_OFF_AUTO_OFF) {
> -        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
> -        /* Any error other than -ENOTSUP(board's MSI support is broken)
> -         * is a programming error */
> -        assert(!ret || ret == -ENOTSUP);
> -        if (ret && xhci->msi == ON_OFF_AUTO_ON) {
> -            /* Can't satisfy user's explicit msi=on request, fail */
> -            error_append_hint(&err, "You have to use msi=auto (default) or "
> -                    "msi=off with this machine type.\n");
> -            error_propagate(errp, err);
> -            return;
> -        }
> -        assert(!err || xhci->msi == ON_OFF_AUTO_AUTO);
> -        /* With msi=auto, we fall back to MSI off silently */
> -        error_free(err);
> -    }
> -

Outside this patch's scope, but here goes anyway:

>      if (xhci->numintrs > MAXINTRS) {
>          xhci->numintrs = MAXINTRS;
>      }
       while (xhci->numintrs & (xhci->numintrs - 1)) {   /* ! power of 2 */
           xhci->numintrs++;
       }
       if (xhci->numintrs < 1) {
           xhci->numintrs = 1;
       }
       if (xhci->numslots > MAXSLOTS) {
           xhci->numslots = MAXSLOTS;
       }
       if (xhci->numslots < 1) {
           xhci->numslots = 1;
       }
       if (xhci_get_flag(xhci, XHCI_FLAG_ENABLE_STREAMS)) {
           xhci->max_pstreams_mask = 7; /* == 256 primary streams */
       } else {
> @@ -3634,7 +3615,22 @@ static void usb_xhci_realize(struct PCIDevice *dev, 
> Error **errp)
>          xhci->max_pstreams_mask = 0;
>      }

If the user specifies invalid intrs, slots or streams properties, his
configuration is silently corrected to a valid one.  I hate that.
Invalid configuration should be rejected cleanly.

By the way, calling the property "intrs" differs needlessly from virtio,
which calls it "vectors".

Anyway, nothing wrong with the patch here.

Before this patch, we can have msi_init() choke on invalid
xhci->numintrs before we reach the configuration correction code.
Your patch fixes it.

>  
> -    xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, 
> xhci);
> +    if (xhci->msi != ON_OFF_AUTO_OFF) {
> +        ret = msi_init(dev, 0x70, xhci->numintrs, true, false, &err);
> +        /* Any error other than -ENOTSUP(board's MSI support is broken)
> +         * is a programming error */
> +        assert(!ret || ret == -ENOTSUP);
> +        if (ret && xhci->msi == ON_OFF_AUTO_ON) {
> +            /* Can't satisfy user's explicit msi=on request, fail */
> +            error_append_hint(&err, "You have to use msi=auto (default) or "
> +                    "msi=off with this machine type.\n");
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        assert(!err || xhci->msi == ON_OFF_AUTO_AUTO);
> +        /* With msi=auto, we fall back to MSI off silently */
> +        error_free(err);
> +    }
>  
>      memory_region_init(&xhci->mem, OBJECT(xhci), "xhci", LEN_REGS);
>      memory_region_init_io(&xhci->mem_cap, OBJECT(xhci), &xhci_cap_ops, xhci,
> @@ -3664,6 +3660,9 @@ static void usb_xhci_realize(struct PCIDevice *dev, 
> Error **errp)
>                       
> PCI_BASE_ADDRESS_SPACE_MEMORY|PCI_BASE_ADDRESS_MEM_TYPE_64,
>                       &xhci->mem);
>  
> +    usb_xhci_init(xhci);
> +    xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, 
> xhci);
> +

Before your patch, resources allocated by usb_xhci_init() were leaked on
msi_init() failure.

Your patch also delays it and timer_new_ns() until after we can't fail
anymore.  That's a good idea unless their work is actually used earlier.
It isn't as far as I can see.

>      if (pci_bus_is_express(dev->bus) ||
>          xhci_get_flag(xhci, XHCI_FLAG_FORCE_PCIE_ENDCAP)) {
>          ret = pcie_endpoint_cap_init(dev, 0xa0);

I can take this through my tree if Gerd provides at least his Acked-by,
preferably Reviewed-by.  Gerd, if you'd rather take it through yours,
let me know.  I'll take this series into my tree then, wait for the
patch to make its way through yours, and rebase.