Re: [PATCH 5/8] hw/usb/xhci: Move HCD constants to a header and add register constants

[Nicholas Piggin]

On Thu Dec 19, 2024 at 1:08 AM AEST, Phil Dennis-Jordan wrote:
> This looks sensible to me overall.
>
> For the new symbolic constants for MMIO register offsets such as
> XHCI_OPER_*, XHCI_INTR_* and so on, I'm wondering if it would be clearer to
> give them all an _OFFSET suffix. It's not perfectly consistent to do so
> across the code base, but quite a few device types do follow that
> convention. In my opinion it improves readability, especially in the header
> file, where these offset constants are frequently mixed in with constant
> values that can be written to or read from these registers.

I'm not strongly attached. I slightly prefer suffix, like
XHCI_HCCAP_REG_CAPLENGTH) for MMIO regs, so all the common
part of the name lines up.

Should the entire codebase have a guideline for memory offset
naming conventions? Or does it not matter that much.

[snip]

> > -    case 0x0c: /* HCSPARAMS 3 */
> > +    case XHCI_HCCAP_HCSPARAMS3:
> >          ret = 0x00000000;
> >          break;
> > -    case 0x10: /* HCCPARAMS */
> > -        if (sizeof(dma_addr_t) == 4) {
> > -            ret = 0x00080000 | (xhci->max_pstreams_mask << 12);
> > -        } else {
> > -            ret = 0x00080001 | (xhci->max_pstreams_mask << 12);
> > +    case XHCI_HCCAP_HCCPARAMS1:
> > +        ret = (XHCI_HCCAP_EXTCAP_START >> 2) | (xhci->max_pstreams_mask
> > << 12);
> >
>
> This doesn't look like it's equivalent to the original code. I think you
> want
> ((XHCI_HCCAP_EXTCAP_START >> 2) << 16) | (xhci->max_pstreams_mask << 12);

Good catch.

>
> That's… not particularly readable either though, so if we're going to break
> up the magic numbers here, how about something like:
>
> ret = (XHCI_HCCAP_EXTCAP_START / 4) << XHCI_HCCPARAM_EXTPTR_SHIFT;
> ret |= xhci->max_pstreams_mask << XHCI_HCCPARAM_MAXPSASIZE_SHIFT;
>
>
> > +        if (sizeof(dma_addr_t) == 8) {
> > +            ret |= 0x00000001; /* AC64 */
> >
>
> and then this can become
> ret |= XHCI_HCCPARAM_AC64;
>
> or something like that.

Sure.

[snip]

> >      switch (reg) {
> > -    case 0x00: /* PORTSC */
> > +    case XHCI_PORT_PORTSC:
> >          ret = port->portsc;
> >          break;
> > -    case 0x04: /* PORTPMSC */
> > -    case 0x08: /* PORTLI */
> > +    case XHCI_PORT_PORTPMSC:
> > +    case XHCI_PORT_PORTLI:
> >          ret = 0;
> >          break;
> > -    case 0x0c: /* reserved */
> >
>
> I think it's worth keeping explicitly unhandled case labels documented like
> this. (This one appears to be XHCI_PORT_PORTHLPMC nowadays, I assume it was
> reserved in an earlier spec version.)

Okay.

>      default:
> >          trace_usb_xhci_unimplemented("port read", reg);
> >          ret = 0;
> > @@ -2829,7 +2668,7 @@ static void xhci_port_write(void *ptr, hwaddr reg,
> >      trace_usb_xhci_port_write(port->portnr, reg, val);
> >
> >      switch (reg) {
> > -    case 0x00: /* PORTSC */
> > +    case XHCI_PORT_PORTSC:
> >          /* write-1-to-start bits */
> >          if (val & PORTSC_WPR) {
> >              xhci_port_reset(port, true);
> > @@ -2880,8 +2719,6 @@ static void xhci_port_write(void *ptr, hwaddr reg,
> >              xhci_port_notify(port, notify);
> >          }
> >          break;
> > -    case 0x04: /* PORTPMSC */
> > -    case 0x08: /* PORTLI */
> >
>
> Hmm. Looks like PORTLI is actually a read-only register, so writing this
> ought to trigger a LOG_GUEST_ERROR. And I don't think it's a bad thing to
> explicitly document PORTPMSC as unimplemented. (And I guess that ought to
> be a LOG_UNIMP, not a trace, sigh.) The improved logging can be a separate
> commit - in fact I don't mind tagging that fix onto my own pending XHCI
> patch set, but I think for this commit we ought to keep the case labels
> (with the new symbolic constants).

Okay I'll add back the reserved cases.

Yeah, memory access handling in drivers is really inconsistent all over
the tree. It would be nice if there was some helpers or something that
added common template for access tracing, unimp and guest error logs,
etc.

Thanks,
Nick