Re: [PATCH 12/12] hw/xen: add support for Xen primary console in emulated mode

[Paul Durrant <xadimgnik@gmail.com>]

On 24/10/2023 17:34, David Woodhouse wrote:
> On Tue, 2023-10-24 at 17:25 +0100, Paul Durrant wrote:
>> On 24/10/2023 16:49, David Woodhouse wrote:
>>> On Tue, 2023-10-24 at 16:39 +0100, Paul Durrant wrote:
>>>> On 24/10/2023 16:37, David Woodhouse wrote:
>>>>> On Tue, 2023-10-24 at 15:20 +0100, Paul Durrant wrote:
>>>>>> On 16/10/2023 16:19, David Woodhouse wrote:
>>>>>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>>>>>
>>>>>>> The primary console is special because the toolstack maps a page at a
>>>>>>> fixed GFN and also allocates the guest-side event channel. Add support
>>>>>>> for that in emulated mode, so that we can have a primary console.
>>>>>>>
>>>>>>> Add a *very* rudimentary stub of foriegnmem ops for emulated mode, which
>>>>>>> supports literally nothing except a single-page mapping of the console
>>>>>>> page. This might as well have been a hack in the xen_console driver, but
>>>>>>> this way at least the special-casing is kept within the Xen emulation
>>>>>>> code, and it gives us a hook for a more complete implementation if/when
>>>>>>> we ever do need one.
>>>>>>>
>>>>>> Why can't you map the console page via the grant table like the xenstore
>>>>>> page?
>>>>>
>>>>> I suppose we could, but I didn't really want the generic xen-console
>>>>> device code having any more of a special case for 'Xen emulation' than
>>>>> it does already by having to call xen_primary_console_create().
>>>>>
>>>>
>>>> But doesn't is save you the whole foreignmem thing? You can use the
>>>> grant table for primary and secondary consoles.
>>>
>>> Yes. And I could leave the existing foreignmem thing just for the case
>>> of primary console under true Xen. It's probably not that awful a
>>> special case, in the end.
>>>
>>> Then again, I was surprised I didn't *already* have a foreignmem ops
>>> for the emulated case, and we're probably going to want to continue
>>> fleshing it out later, so I don't really mind adding it.
>>>
>>
>> True. We'll need it for some of the other more fun protocols like vkbd
>> or fb. Still, I think it'd be nicer to align the xenstore and primary
>> console code to look similar and punt the work until then :-)
> 
> I don't think it ends up looking like xenstore either way, does it?
> Xenstore is special because it gets to use the original pointer to its
> own page.
> 

Not sure what you mean there? A guest can query the PFN for either 
xenstore or console using HVM params, or it can find them in its own 
grant table entries 0 or 1.

> I don't think I want to hack the xen_console code to explicitly call a
> xen_console_give_me_your_page() function. If not foreignmem, I think
> you were suggesting that we actually call the grant mapping code to get
> a pointer to the underlying page, right?

I'm suggesting that the page be mapped in the same way that the xenstore 
backend does:

1462    /* 

1463     * We don't actually access the guest's page through the grant, 
because
1464     * this isn't real Xen, and we can just use the page we gave it 
in the
1465     * first place. Map the grant anyway, mostly for cosmetic 
purposes so
1466     * it *looks* like it's in use in the guest-visible grant table. 

1467     */
1468    s->gt = qemu_xen_gnttab_open();
1469    uint32_t xs_gntref = GNTTAB_RESERVED_XENSTORE;
1470    s->granted_xs = qemu_xen_gnttab_map_refs(s->gt, 1, xen_domid, 
&xs_gntref,
1471                                             PROT_READ | PROT_WRITE);


> 
> I could kind of live with that... except that Xen has this ugly
> convention that the "ring-ref" frontend node for the primary console
> actually has the *MFN* not a grant ref. Which I don't understand since
> the toolstack *does* populate the grant table for it (just as it does
> for the xenstore page). But we'd have to add a special case exception
> to that special case, so that in the emu case it's an actual grant ref
> again. I think I prefer just having a stub of foreignmem, TBH.
> 

You're worried about the guest changing the page it uses for the primary 
console and putting a new one in xenstore? I'd be amazed if that even 
works on Xen unless the guest is careful to write it into 
GNTTAB_RESERVED_CONSOLE.

> (I didn't yet manage to get Xen to actually create a primary console of
> type iomem, FWIW)
> 

No, that doesn't entirely surprise me.

   Paul