Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.08.2022 15:43, Marek Marczykowski-Górecki wrote:
> On Thu, Aug 04, 2022 at 02:57:49PM +0200, Jan Beulich wrote:
>> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
>>> +struct dbc {
>>> +    struct dbc_reg __iomem *dbc_reg;
>>> +    struct xhci_dbc_ctx *dbc_ctx;
>>> +    struct xhci_erst_segment *dbc_erst;
>>> +    struct xhci_trb_ring dbc_ering;
>>> +    struct xhci_trb_ring dbc_oring;
>>> +    struct xhci_trb_ring dbc_iring;
>>> +    struct dbc_work_ring dbc_owork;
>>> +    struct xhci_string_descriptor *dbc_str;
>>
>> I'm afraid I still don't see why the static page this field is being
>> initialized with is necessary. Can't you have a static variable (of
>> some struct type) all pre-filled at build time, which you then apply
>> virt_to_maddr() to in order to fill the respective dbc_ctx fields?
> 
> I need to keep this structure somewhere DMA-reachable for the device (as
> in - included in appropriate IOMMU context). Patch 8/10 is doing it. And
> also, patch 8/10 is putting it together with other DMA-reachable
> structures (not a separate page on its own). If I'd make it a separate
> static variable (not part of that later struct), I'd need to reserve the
> whole page for it - to guarantee no unrelated data lives on the same
> (DMA-reachable) page.
> 
> As for statically initializing it, if would require the whole
> (multi-page DMA-reachable) thing living in .data, not .bss, so a bigger
> binary (not a huge concern due to compression, but still). But more
> importantly, I don't know how to do it in a readable way, and you have
> complained about readability of initializer of this structure in v2.
> 
>> That struct will be quite a bit less than a page's worth in size.
> 
> See above - it cannot share page with unrelated Xen data.

I have to admit that I'd see no issue if these lived side by side with
e.g. other string literals. The more that the device is supposed to be
exposed to Dom0 only anyway, and hence that'll be the only domain able
to get at that data.

>> If you build the file with -fshort-wchar, you may even be able to
>> use easy to read string literals for the initializer.
> 
> I can try, but I'm not exactly sure how to make readable UTF-16
> literals...

L"Xen" looks sufficiently readable to me. We use this all over the
place in the EFI interfacing code.

Jan