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

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 6390 bytes --]

On Wed, 2023-10-25 at 09:31 +0100, Paul Durrant wrote:
> 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.

The code in our xen_xenstore.c uses its *own* pointer (s->xs) to the
MemoryRegion that it created (s->xenstore_page). It is its own backend,
as well as doing the "magic" to create the guest-side mapping and event
channel.

The difference for the console code is that we actually have a
*separation* between the standard backend code in xen_console.c, and
the magic frontend parts for the emulated mode.


> 
> > 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);

It already *is*. But as with xen_xenstore.c, nothing ever *uses* the
s->granted_xs pointer. It's just cosmetic to make the grant table look
right.

But that doesn't help the *backend* code. The backend doesn't even know
the grant ref#, because the convention we inherited from Xen is that
the `ring-ref` in XenStore for the primary console is actually the MFN,
to be mapped as foreignmem.

Of course, we *do* know the grant-ref for the primary console, as it's
always GNTTAB_RESERVED_CONSOLE. So I suppose we could put a hack into
the xen_console backend to map *that* in the case of primary console
under emu? In fact that would probably do the right thing even under
Xen if we could persuade Xen to make an ioemu primary console?





> > 
> > 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.

Not worried about the guest changing it. I was mostly just concerned
about the xen-console having to have another special case and magically
"know" it. But I suppose I can live with it being hard-coded to
GNTTAB_RESERVED_CONSOLE. I'll knock that up and see how it makes me
feel.

I'm reworking some of that connect/disconnect code anyway, to have the
backend tell the primary_console code directly what the backend port#
is, so I can remove the soft-reset hacks in xen_evtchn.c entirely.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]