[PATCH 0/1] hw/ide/core.c: fix handling of unsupported commands

[PATCH 0/1] hw/ide/core.c: fix handling of unsupported commands

[Mateusz Albecki]

The patch was developed during the debug for the UEFI SCT issue reported in
https://bugzilla.tianocore.org/show_bug.cgi?id=4016. After fixing the issue
in EDK2 the test was still failing on qemu since qemu wouldn't return abort and
instead we would get a command timeout. Additionally qemu also has another bug in that
it reports support for storage security commands which it doesn't support(will file a bug).

Tests:
- tested against UEFI SCT test mentioned in bugzilla and it is passing.
- able to read/write files on ahci controller from UEFI.

Mateusz Albecki (1):
  hw/ide/core.c: fix handling of unsupported commands

 hw/ide/core.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.40.0

[PATCH 1/1] hw/ide/core.c: fix handling of unsupported commands

[Mateusz Albecki]

From: Mateusz Albecki <mateusz.albecki@outlook.com>

Current code will not call ide_cmd_done when aborting the unsupported
command which will lead to the command timeout on the driver side instead
of getting a D2H FIS with ABRT indication. This can lead to problems on the
driver side as the spec mandates that device should return a D2H FIS with
ABRT bit set in ERR register(from SATA 3.1 section 16.3.3.8.6)

Signed-off-by: Mateusz Albecki <mateusz.p.albecki@gmail.com>
---
 hw/ide/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 45d14a25e9..d7027bbd4d 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2146,6 +2146,7 @@ void ide_bus_exec_cmd(IDEBus *bus, uint32_t val)
 
     if (!ide_cmd_permitted(s, val)) {
         ide_abort_command(s);
+        ide_cmd_done(s);
         ide_bus_set_irq(s->bus);
         return;
     }
-- 
2.40.0

Re: [PATCH 1/1] hw/ide/core.c: fix handling of unsupported commands

[John Snow]

On Sun, Apr 16, 2023 at 6:29 PM Mateusz Albecki
<mateusz.p.albecki@gmail.com> wrote:
>
> From: Mateusz Albecki <mateusz.albecki@outlook.com>
>
> Current code will not call ide_cmd_done when aborting the unsupported
> command which will lead to the command timeout on the driver side instead
> of getting a D2H FIS with ABRT indication. This can lead to problems on the
> driver side as the spec mandates that device should return a D2H FIS with
> ABRT bit set in ERR register(from SATA 3.1 section 16.3.3.8.6)
>
> Signed-off-by: Mateusz Albecki <mateusz.p.albecki@gmail.com>
> ---
>  hw/ide/core.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 45d14a25e9..d7027bbd4d 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2146,6 +2146,7 @@ void ide_bus_exec_cmd(IDEBus *bus, uint32_t val)
>
>      if (!ide_cmd_permitted(s, val)) {
>          ide_abort_command(s);
> +        ide_cmd_done(s);
>          ide_bus_set_irq(s->bus);
>          return;
>      }
> --
> 2.40.0
>

I recently noticed that Niklas Cassel sent a patch to fix unsupported
command handling:
https://lists.gnu.org/archive/html/qemu-devel/2023-04/msg05552.html

I suspect that his approach is the more technically correct one and
that calling ide_cmd_done here is a heavy cudgel that may have
unintended consequences. Am I mistaken?
Can you check that Niklas's patch solves your issue? I think you're
both solving the same problem. I've CC'd him on this patch as well.

--js

Re: [PATCH 1/1] hw/ide/core.c: fix handling of unsupported commands

[Mateusz Albecki]

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

Certainly seems like my patch is wrong as it will make the abort path
execute ide_cmd_done twice. During debug I came to the conclusion that
ide_cmd_done is not called at all as I was getting timeouts on the driver
side while waiting for D2H FIS. I am still not sure how I was getting this
behavior if the problem was actually with setting correct error bits. Even
so I think it can be safely assumed that Niklas' change will solve the
issue, I will try to verify it in a couple of days and if I see any problem
I will come back to you.

Mateusz

On Wed, 17 May 2023 at 23:33, John Snow <jsnow@redhat.com> wrote:

> On Sun, Apr 16, 2023 at 6:29 PM Mateusz Albecki
> <mateusz.p.albecki@gmail.com> wrote:
> >
> > From: Mateusz Albecki <mateusz.albecki@outlook.com>
> >
> > Current code will not call ide_cmd_done when aborting the unsupported
> > command which will lead to the command timeout on the driver side instead
> > of getting a D2H FIS with ABRT indication. This can lead to problems on
> the
> > driver side as the spec mandates that device should return a D2H FIS with
> > ABRT bit set in ERR register(from SATA 3.1 section 16.3.3.8.6)
> >
> > Signed-off-by: Mateusz Albecki <mateusz.p.albecki@gmail.com>
> > ---
> >  hw/ide/core.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index 45d14a25e9..d7027bbd4d 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -2146,6 +2146,7 @@ void ide_bus_exec_cmd(IDEBus *bus, uint32_t val)
> >
> >      if (!ide_cmd_permitted(s, val)) {
> >          ide_abort_command(s);
> > +        ide_cmd_done(s);
> >          ide_bus_set_irq(s->bus);
> >          return;
> >      }
> > --
> > 2.40.0
> >
>
> I recently noticed that Niklas Cassel sent a patch to fix unsupported
> command handling:
> https://lists.gnu.org/archive/html/qemu-devel/2023-04/msg05552.html
>
> I suspect that his approach is the more technically correct one and
> that calling ide_cmd_done here is a heavy cudgel that may have
> unintended consequences. Am I mistaken?
> Can you check that Niklas's patch solves your issue? I think you're
> both solving the same problem. I've CC'd him on this patch as well.
>
> --js
>
>

[-- Attachment #2: Type: text/html, Size: 3098 bytes --]

Re: [PATCH 1/1] hw/ide/core.c: fix handling of unsupported commands

[John Snow]

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

On Mon, May 22, 2023 at 5:16 PM Mateusz Albecki <mateusz.p.albecki@gmail.com>
wrote:
>
> Certainly seems like my patch is wrong as it will make the abort path
execute ide_cmd_done twice. During debug I came to the conclusion that
ide_cmd_done is not called at all as I was getting timeouts on the driver
side while waiting for D2H FIS. I am still not sure how I was getting this
behavior if the problem was actually with setting correct error bits. Even
so I think it can be safely assumed that Niklas' change will solve the
issue, I will try to verify it in a couple of days and if I see any problem
I will come back to you.
>
> Mateusz

Great, thanks :)

I'm waiting to hear back from Niklas, but I'm hoping to take their patches
this cycle as I think they look quite good.

Thank you for submitting bug reports and patches :~)

--js

[-- Attachment #2: Type: text/html, Size: 1171 bytes --]

Re: [PATCH 1/1] hw/ide/core.c: fix handling of unsupported commands

[Niklas Cassel]

On Tue, May 23, 2023 at 10:35:56AM -0400, John Snow wrote:
> On Mon, May 22, 2023 at 5:16???PM Mateusz Albecki <mateusz.p.albecki@gmail.com>
> wrote:
> >
> > Certainly seems like my patch is wrong as it will make the abort path
> execute ide_cmd_done twice. During debug I came to the conclusion that
> ide_cmd_done is not called at all as I was getting timeouts on the driver
> side while waiting for D2H FIS. I am still not sure how I was getting this
> behavior if the problem was actually with setting correct error bits. Even
> so I think it can be safely assumed that Niklas' change will solve the
> issue, I will try to verify it in a couple of days and if I see any problem
> I will come back to you.
> >
> > Mateusz
> 
> Great, thanks :)
> 
> I'm waiting to hear back from Niklas, but I'm hoping to take their patches
> this cycle as I think they look quite good.

Hello John,

Unfortunately, I've noticed an increase in boot time during
the initial SeaBIOS part of QEMU with my changes.

Will need to debug to see which change is causing this.

I'm at a conference this week, so it might take until next
week until I have time to figure out why this is happening.

So unfortunately, I think we need to hold off with my series
for now.

If Mateusz can confirm that
https://lists.gnu.org/archive/html/qemu-devel/2023-04/msg05552.html
solves his issue, and that it does not cause an increased boot
time for SeaBIOS, perhaps that patch could be picked up separately.


Kind regards,
Niklas

Re: [PATCH 1/1] hw/ide/core.c: fix handling of unsupported commands

[John Snow]

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

On Wed, May 24, 2023, 4:17 AM Niklas Cassel <nks@flawful.org> wrote:

> On Tue, May 23, 2023 at 10:35:56AM -0400, John Snow wrote:
> > On Mon, May 22, 2023 at 5:16???PM Mateusz Albecki <
> mateusz.p.albecki@gmail.com>
> > wrote:
> > >
> > > Certainly seems like my patch is wrong as it will make the abort path
> > execute ide_cmd_done twice. During debug I came to the conclusion that
> > ide_cmd_done is not called at all as I was getting timeouts on the driver
> > side while waiting for D2H FIS. I am still not sure how I was getting
> this
> > behavior if the problem was actually with setting correct error bits.
> Even
> > so I think it can be safely assumed that Niklas' change will solve the
> > issue, I will try to verify it in a couple of days and if I see any
> problem
> > I will come back to you.
> > >
> > > Mateusz
> >
> > Great, thanks :)
> >
> > I'm waiting to hear back from Niklas, but I'm hoping to take their
> patches
> > this cycle as I think they look quite good.
>
> Hello John,
>
> Unfortunately, I've noticed an increase in boot time during
> the initial SeaBIOS part of QEMU with my changes.
>
> Will need to debug to see which change is causing this.
>
> I'm at a conference this week, so it might take until next
> week until I have time to figure out why this is happening.
>
> So unfortunately, I think we need to hold off with my series
> for now.
>

Shame. Feel free to resend it once you've isolated the problem and I'll try
to fast-track it, since it'd be nice to have my more embarrassing mistakes
for NCQ fixed :)

(If I become hard to reach, please ping me with a direct, non-patch email
so it bubbles up to the top of my inbox.)

((...by the way, what are you working on? What motivates the interest in
AHCI/SATA now? Can you say?))


> If Mateusz can confirm that
> https://lists.gnu.org/archive/html/qemu-devel/2023-04/msg05552.html
> solves his issue, and that it does not cause an increased boot
> time for SeaBIOS, perhaps that patch could be picked up separately.
>
>
> Kind regards,
> Niklas
>
>

[-- Attachment #2: Type: text/html, Size: 3085 bytes --]

Re: [PATCH 1/1] hw/ide/core.c: fix handling of unsupported commands

[Niklas Cassel]

On Wed, May 24, 2023 at 11:20:41AM -0400, John Snow wrote:
> On Wed, May 24, 2023, 4:17 AM Niklas Cassel <nks@flawful.org> wrote:
> 
> > On Tue, May 23, 2023 at 10:35:56AM -0400, John Snow wrote:
> > > On Mon, May 22, 2023 at 5:16???PM Mateusz Albecki <
> > mateusz.p.albecki@gmail.com>
> > > wrote:
> > > >
> > > > Certainly seems like my patch is wrong as it will make the abort path
> > > execute ide_cmd_done twice. During debug I came to the conclusion that
> > > ide_cmd_done is not called at all as I was getting timeouts on the driver
> > > side while waiting for D2H FIS. I am still not sure how I was getting
> > this
> > > behavior if the problem was actually with setting correct error bits.
> > Even
> > > so I think it can be safely assumed that Niklas' change will solve the
> > > issue, I will try to verify it in a couple of days and if I see any
> > problem
> > > I will come back to you.
> > > >
> > > > Mateusz
> > >
> > > Great, thanks :)
> > >
> > > I'm waiting to hear back from Niklas, but I'm hoping to take their
> > patches
> > > this cycle as I think they look quite good.
> >
> > Hello John,
> >
> > Unfortunately, I've noticed an increase in boot time during
> > the initial SeaBIOS part of QEMU with my changes.
> >
> > Will need to debug to see which change is causing this.
> >
> > I'm at a conference this week, so it might take until next
> > week until I have time to figure out why this is happening.
> >
> > So unfortunately, I think we need to hold off with my series
> > for now.
> >
> 
> Shame. Feel free to resend it once you've isolated the problem and I'll try
> to fast-track it, since it'd be nice to have my more embarrassing mistakes
> for NCQ fixed :)
> 
> (If I become hard to reach, please ping me with a direct, non-patch email
> so it bubbles up to the top of my inbox.)
> 
> ((...by the way, what are you working on? What motivates the interest in
> AHCI/SATA now? Can you say?))

Hello John,

It is simple, Western Digital is still selling a lot of SATA drives :)

And we used QEMU to verify some of the support for Command Duration Limits
in linux:
https://lore.kernel.org/linux-scsi/20230511011356.227789-1-nks@flawful.org/

Right now, I don't intend to upstream all my hacky patches to QEMU,
just the things that might be useful for everyone, e.g. GPL and NCQ error log
(and possibly Sense Data Reporting feature set support, and NCQ autosense).


Kind regards,
Niklas

Re: [PATCH 1/1] hw/ide/core.c: fix handling of unsupported commands

[John Snow]

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

On Thu, Jun 1, 2023, 10:03 AM Niklas Cassel <Niklas.Cassel@wdc.com> wrote:

> On Wed, May 24, 2023 at 11:20:41AM -0400, John Snow wrote:
> > On Wed, May 24, 2023, 4:17 AM Niklas Cassel <nks@flawful.org> wrote:
> >
> > > On Tue, May 23, 2023 at 10:35:56AM -0400, John Snow wrote:
> > > > On Mon, May 22, 2023 at 5:16???PM Mateusz Albecki <
> > > mateusz.p.albecki@gmail.com>
> > > > wrote:
> > > > >
> > > > > Certainly seems like my patch is wrong as it will make the abort
> path
> > > > execute ide_cmd_done twice. During debug I came to the conclusion
> that
> > > > ide_cmd_done is not called at all as I was getting timeouts on the
> driver
> > > > side while waiting for D2H FIS. I am still not sure how I was getting
> > > this
> > > > behavior if the problem was actually with setting correct error bits.
> > > Even
> > > > so I think it can be safely assumed that Niklas' change will solve
> the
> > > > issue, I will try to verify it in a couple of days and if I see any
> > > problem
> > > > I will come back to you.
> > > > >
> > > > > Mateusz
> > > >
> > > > Great, thanks :)
> > > >
> > > > I'm waiting to hear back from Niklas, but I'm hoping to take their
> > > patches
> > > > this cycle as I think they look quite good.
> > >
> > > Hello John,
> > >
> > > Unfortunately, I've noticed an increase in boot time during
> > > the initial SeaBIOS part of QEMU with my changes.
> > >
> > > Will need to debug to see which change is causing this.
> > >
> > > I'm at a conference this week, so it might take until next
> > > week until I have time to figure out why this is happening.
> > >
> > > So unfortunately, I think we need to hold off with my series
> > > for now.
> > >
> >
> > Shame. Feel free to resend it once you've isolated the problem and I'll
> try
> > to fast-track it, since it'd be nice to have my more embarrassing
> mistakes
> > for NCQ fixed :)
> >
> > (If I become hard to reach, please ping me with a direct, non-patch email
> > so it bubbles up to the top of my inbox.)
> >
> > ((...by the way, what are you working on? What motivates the interest in
> > AHCI/SATA now? Can you say?))
>
> Hello John,
>
> It is simple, Western Digital is still selling a lot of SATA drives :)
>

Haha! Fair enough. I built my first computer without a single AHCI device
in it last year, but you can't say the same for my Synology downstairs in
my networking closet :)


> And we used QEMU to verify some of the support for Command Duration Limits
> in linux:
> https://lore.kernel.org/linux-scsi/20230511011356.227789-1-nks@flawful.org/


Ah-ha, that helps explain it :)

If you find other problems, even if you don't intend to fix them, please
file a gitlab issue to document that there's work that needs done.


>
> Right now, I don't intend to upstream all my hacky patches to QEMU,
> just the things that might be useful for everyone, e.g. GPL and NCQ error
> log
> (and possibly Sense Data Reporting feature set support, and NCQ autosense).
>

Got it, thanks! I'll try to set aside time to look over your patches
"soon", I'd like then for 8.1. If I go quiet, *please* ping me.


>
> Kind regards,
> Niklas
>

[-- Attachment #2: Type: text/html, Size: 5059 bytes --]