Re: [PATCH 0/5] serial: Add MMIO & SPCR support for AWS EC2 metal instances

[Glenn Washburn <development@efficientek.com>]

On Fri, 19 Mar 2021 12:27:15 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Thu, 2021-03-18 at 19:03 -0500, Glenn Washburn wrote:
> > > This was tested using SPCR on an actual c5.metal instance, and
> > > using explicit instanciation via serial -p mmioXXXXXXXX on a
> > > modified qemu hacked to create MMIO PCI serial ports.
> > 
> > 
> > When you say a modified qemu, was that a source level change? I'm
> > curious how hard it would be to add this test to the current GRUB
> > make check tests (many of which already use qemu). Of course, if
> > they source of qemu was modified, then its probably a deal breaker
> > (until it could be accepted upstream). 
> 
> Yes, I added an "mmio" option to pci-serial that makes register with
> the Amazon UART vid/did and use an MMIO BAR :-) It's a bit of a hack
> but I can try upstreaming it.

I may have been confusing above. When I said deal breaker, I didn't
mean for the patch series. I meant if it was source changes to qemu (as
it is), it would be a deal breaker to require that test. It would be
ideal to try to upstream the change to qemu and then get a test working
here, but it shouldn't be a requirement to include this patch series.

> > Also I haven't looked into it, but seems like it might not be hard
> > to add a separate test for the part using the SPCR table via qemu
> > (perhaps using the "-acpitable" arg).
> 
> My experience with ACPI is extremely limited, I've never done such
> things as build tables etc... but I can certainly try to look into it
> as time permits.

Ok, I was hoping it might be low hanging fruit. As it seems like it
might not be, I don't think not having this test should be a blocker.
But it would be nice if you could take a quick look at it to see if it
would in fact be easy.

> > Also, could you add to the documentation on the usage of these
> > changes?
> 
> I added some documentation to the "serial" command syntax change for
> MMIO. I didn't add anything about SPCR indeed, where do you suggest I
> add it ? Same spot ?

Yes, at the end of that paragraph an additional sentence seems like a
good place. I'll add a comment on some changes to the --mmio doc change
in that patch.

> > New functionality should in general come with new tests (when
> > feasible)
> 
> Yes, I understand, it's just that in this specific case it's rather
> hard ... :-) I'll see what I can do with qemu but the best case
> scenario would involve upstreaming something there and then depending
> on that change trickling down to be able to use it in the grub tests
> which would be ... tricky....

Yep, I'm not suggesting we add any tests that rely on changes not in
a qemu release. 

> > and additions to the documentation.
> 
> Right, I did a bit, I can look into adding more, let me know if there
> are other parts of the doc you wish me to update.

Ok, so to sum up, I don't think these changes should be blocked by not
having tests. There still needs to be a proper code review, which I'm
no comfortable doing since its not really my area.

Glenn