Re: [Virtio-fs] [PATCH] vhost-user-fs: fix features handling
On 09/04/2021 18:56, Vivek Goyal wrote:
On Thu, Apr 08, 2021 at 10:55:34PM +0300, Anton Kuchin wrote:
Make virtio-fs take into account server capabilities.
Just returning requested features assumes they all of then are implemented
by server and results in setting unsupported configuration if some of them
are absent.
Signed-off-by: Anton Kuchin
[CC stefan and qemu-devel.]
Can you give more details of what problem exactly you are facing. Or
this fix is about avoiding a future problem where device can refuse
to support a feature qemu is requesting for.
This fixes existing problem that qemu ignores features (un)supported by
backend and this works fine only if backend features match features of
qemu. Otherwise qemu incorrectly assumes that backend suports all of
them and calls vhost_set_features() not taking into account result of
previous vhost_get_features() call. This breaks protocol and can crash
server or cause incorrect behavior.
This is why I hope it to be accepted in time for 6.0 release.
IIUC, this patch is preparing a list of features vhost-user-fs device
can support. Then it calls vhost_get_features() which makes sure that
all these features are support by real vhost-user device (hdev->features).
If not, then corresponding feature is reset and remaining features
are returned to caller.
When this callback is executed in virtio_bus_device_plugged() list of
features that vhost-backend supports has been already obtained earlier
by vhost_user_get_features() in vuf_device_realize() and stored in
hdev->features. vuf_get_features() should return bitmask of features
that are common for vhost backend (hdev->features) and frontend
(vdev->host_features). But instead it just returns host features.
This feature negotion bit is called in so many places that I am kind of
lost that who should be doing what. Will leave it to Stefan who
understands it much better.
---
hw/virtio/vhost-user-fs.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index ac4fc34b36..6cf983ba0e 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -24,6 +24,14 @@
#include "monitor/monitor.h"
#include "sysemu/sysemu.h"
+static const int user_feature_bits[] = {
+VIRTIO_F_VERSION_1,
+VIRTIO_RING_F_INDIRECT_DESC,
+VIRTIO_RING_F_EVENT_IDX,
+VIRTIO_F_NOTIFY_ON_EMPTY,
+VHOST_INVALID_FEATURE_BIT
+};
+
static void vuf_get_config(VirtIODevice *vdev, uint8_t *config)
{
VHostUserFS *fs = VHOST_USER_FS(vdev);
@@ -129,11 +137,12 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t
status)
}
static uint64_t vuf_get_features(VirtIODevice *vdev,
- uint64_t requested_features,
- Error **errp)
+ uint64_t features,
Will it make sense to keep the name requested_features. This kind of
makes it clear that caller is requesting these features.
I feel there should be few lines of comments also to make it clear
what this function is actually doing.
This fix was inspired by similar functions in
hw/scsi/vhost-scsi-common.c, hw/virtio/vhost-user-vsock.c,
hw/block/vhost-user-blk.c and hw/net/vhost_net.c and I borrowed naming
from them just to be consistent.
IMO this looks like a good place for refactoring because we have more
and more vhost-user devices that duplicate that code, but it can be done
after the bug is fixed.
Vivek
+ Error **errp)
{
-/* No feature bits used yet */
-return requested_features;
+VHostUserFS *fs = VHOST_USER_FS(vdev);
+
+return vhost_get_features(&fs->vhost_dev, user_feature_bits, features);
}
static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq)
--
2.25.1
___
Virtio-fs mailing list
virtio...@redhat.com
https://listman.redhat.com/mailman/listinfo/virtio-fs
Re: [PATCH v2 5/8] target/riscv: Implementation of enhanced PMP (ePMP)
On Fri, Apr 9, 2021 at 10:33 AM Bin Meng wrote: > > Hi Alistair, > > On Fri, Apr 9, 2021 at 8:23 PM Alistair Francis > wrote: > > > > From: Hou Weiying > > > > This commit adds support for ePMP v0.9.1. > > > > The ePMP spec can be found in: > > https://docs.google.com/document/d/1Mh_aiHYxemL0umN3GTTw8vsbmzHZ_nxZXgjgOUzbvc8 > > > > Signed-off-by: Hongzheng-Li > > Signed-off-by: Hou Weiying > > Signed-off-by: Myriad-Dreamin > > Message-Id: > > > > [ Changes by AF: > > - Rebase on master > > - Update to latest spec > > - Use a switch case to handle ePMP MML permissions > > - Fix a few bugs > > ] > > Signed-off-by: Alistair Francis > > --- > > target/riscv/pmp.c | 165 + > > 1 file changed, 153 insertions(+), 12 deletions(-) > > > > It looks like the v1 comments are not addressed? You are right. I sent this before I saw your comments for v1. I will address those comments in a v3. Sorry about that. Alistair > > Regards, > Bin
[PATCH] target/riscv: fix wfi exception behavior
The wfi exception trigger behavior was not taking into account the fact
that user mode is not allowed to execute wfi instructions or the effect
of the hstatus.vtw bit. It was also always generating virtual instruction
exceptions when this should only happen when the wfi instruction is
executed when the virtualization mode is enabled:
- when a wfi instruction is executed, an illegal exception should be triggered
if either the current mode is user or the mode is supervisor and mstatus.tw is
set.
- a virtual instruction exception should be raised when a wfi is executed from
virtual-user or virtual-supervisor and hstatus.vtw is set.
Signed-off-by: Jose Martins
---
target/riscv/cpu_bits.h | 1 +
target/riscv/op_helper.c | 8 +---
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 24b24c69c5..ed8b97c788 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -436,6 +436,7 @@
#define HSTATUS_HU 0x0200
#define HSTATUS_VGEIN0x0003F000
#define HSTATUS_VTVM 0x0010
+#define HSTATUS_VTW 0x0020
#define HSTATUS_VTSR 0x0040
#if defined(TARGET_RISCV64)
#define HSTATUS_VSXL0x3
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index d55def76cf..354e39ec26 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -174,9 +174,11 @@ void helper_wfi(CPURISCVState *env)
{
CPUState *cs = env_cpu(env);
-if ((env->priv == PRV_S &&
-get_field(env->mstatus, MSTATUS_TW)) ||
-riscv_cpu_virt_enabled(env)) {
+if ((!riscv_cpu_virt_enabled(env) && env->priv == PRV_U) ||
+(env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TW))) {
+riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
+} else if (riscv_cpu_virt_enabled(env) && (env->priv == PRV_U ||
+(env->priv == PRV_S && get_field(env->hstatus, HSTATUS_VTW {
riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, GETPC());
} else {
cs->halted = 1;
--
2.25.1
Re: [PATCH 1/1] Set TARGET_PAGE_BITS to be 10 instead of 8 bits
Please review.
On Tue, Mar 23, 2021 at 10:28 PM Michael Rolnik wrote:
> If I set TARGET_PAGE_BITS to 12 this *assert assert(v_l2_levels >= 0);*
> will fail (page_table_config_init function) because
> TARGET_PHYS_ADDR_SPACE_BITS is 24 bits, because AVR has 24 is the longest
> pointer AVR has. I can set TARGET_PHYS_ADDR_SPACE_BITS to 32 and
> TARGET_PAGE_BITS to 12 and everything will work fine.
> What do you think?
>
> btw, wrote the original comment, you David referred to, when I did not
> know that QEMU could map several regions to the same page, which is not
> true. That's why I could change 8 to 10.
>
> On Tue, Mar 23, 2021 at 10:11 PM Michael Rolnik wrote:
>
>> how long?
>>
>> On Tue, Mar 23, 2021 at 2:46 PM Dr. David Alan Gilbert <
>> dgilb...@redhat.com> wrote:
>>
>>> * Michael Rolnik (mrol...@gmail.com) wrote:
>>> > Signed-off-by: Michael Rolnik
>>> > ---
>>> > target/avr/cpu-param.h | 8 +---
>>> > target/avr/helper.c| 2 --
>>> > 2 files changed, 1 insertion(+), 9 deletions(-)
>>> >
>>> > diff --git a/target/avr/cpu-param.h b/target/avr/cpu-param.h
>>> > index 7ef4e7c679..9765a9d0db 100644
>>> > --- a/target/avr/cpu-param.h
>>> > +++ b/target/avr/cpu-param.h
>>> > @@ -22,13 +22,7 @@
>>> > #define AVR_CPU_PARAM_H
>>> >
>>> > #define TARGET_LONG_BITS 32
>>> > -/*
>>> > - * TARGET_PAGE_BITS cannot be more than 8 bits because
>>> > - * 1. all IO registers occupy [0x .. 0x00ff] address range, and
>>> they
>>> > - * should be implemented as a device and not memory
>>> > - * 2. SRAM starts at the address 0x0100
>>>
>>> I don't know AVR; but that seems to say why you can't make it any larger
>>> - how do you solve that?
>>>
>>> Dave
>>>
>>> > -#define TARGET_PAGE_BITS 8
>>> > +#define TARGET_PAGE_BITS 10
>>> > #define TARGET_PHYS_ADDR_SPACE_BITS 24
>>> > #define TARGET_VIRT_ADDR_SPACE_BITS 24
>>> > #define NB_MMU_MODES 2
>>> > diff --git a/target/avr/helper.c b/target/avr/helper.c
>>> > index 35e1019594..da658afed3 100644
>>> > --- a/target/avr/helper.c
>>> > +++ b/target/avr/helper.c
>>> > @@ -111,8 +111,6 @@ bool avr_cpu_tlb_fill(CPUState *cs, vaddr address,
>>> int size,
>>> > MemTxAttrs attrs = {};
>>> > uint32_t paddr;
>>> >
>>> > -address &= TARGET_PAGE_MASK;
>>> > -
>>> > if (mmu_idx == MMU_CODE_IDX) {
>>> > /* access to code in flash */
>>> > paddr = OFFSET_CODE + address;
>>> > --
>>> > 2.25.1
>>> >
>>> --
>>> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>>>
>>>
>>
>> --
>> Best Regards,
>> Michael Rolnik
>>
>
>
> --
> Best Regards,
> Michael Rolnik
>
--
Best Regards,
Michael Rolnik
Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
+Mark & Igor On 4/10/21 5:15 PM, Peter Maydell wrote: > On Sat, 10 Apr 2021 at 14:53, Philippe Mathieu-Daudé wrote: >> On 4/10/21 3:19 PM, Luc Michel wrote: >>> Note that clock propagation during reset has always been a complicated >>> problem. Calling clock_propagate is forbidden during the reset's enter >>> phase because of the side effects it can introduce. >> >> Ah... Maybe this is related to the generic reset problem in QEMU :( > > I do wonder if we got the clock-propagation-during-reset part of this > wrong -- it seemed right to me at the time but trying to use the > clock API recently I did run into some unhelpful-seeming results > (I forget the details now, though). > >>> I find your API modification a bit restrictive. I think creating a >>> standalone clock can be useful, e.g. in complicated devices that may >>> want to use internal "intermediate" clocks. I would not remove this >>> possibility to the API users. >> >> Well, this is the point. I can't see a justification to have a clock >> on a non-qdev object. We should be able to model complicated devices >> with qdev. > > The obvious reason is that machine objects are not qdev devices (ie > TYPE_MACHINE inherits directly from TYPE_OBJECT, not from TYPE_DEVICE), > but it's a reasonable thing to say "this machine has a fixed frequency > clock which it connects to the SoC". In this series I restrict Clocks to 1/ qdev and 2/ MachineState (which is the case you described). I tried to think about all the hardware I worked with and all could be somehow modeled as qdev. > I do wonder if the right fix to that would be to make TYPE_MACHINE > be a subtype of TYPE_DEVICE, though -- machines not being subtypes > of device has other annoying effects, like their not having reset > methods or being able to register vmstate structs. There might be > some unanticipated side effects of making that change, though. Yes, that would simplify few things. I might try it. Expanding the topic, this reminds me a discussion between Igor and Mark about MemoryRegion... One was about we can not change the NULL owner to MachineState because the region could be migrated and there is a mismatch. Another was about preparing the design for multi-arch machines, Mark was confuse by having owner for memory regions such DRAM because on a board they aren't owned, can be used by various masters (CPUs, DMA). So the machine should be the owner somehow. Maybe the problem was with migration indeed, I can't remember :S Regards, Phil.
Re: [PULL 00/10] Block layer fixes for 6.0-rc3
On Fri, 9 Apr 2021 at 17:16, Kevin Wolf wrote: > > The following changes since commit ce69aa92d71e13db9c3702a8e8305e8d2463aeb8: > > Merge remote-tracking branch 'remotes/jasowang/tags/net-pull-request' into > staging (2021-04-08 16:45:31 +0100) > > are available in the Git repository at: > > git://repo.or.cz/qemu/kevin.git tags/for-upstream > > for you to fetch changes up to c2c731a4d35062295cd3260e66b3754588a2fad4: > > test-blockjob: Test job_wait_unpaused() (2021-04-09 18:00:29 +0200) > > > Block layer fixes > > - mirror: Fix job-complete race condition causing unexpected errors > - fdc: Fix 'fallback' property on sysbus floppy disk controllers > - rbd: Fix memory leaks > - iotest improvements Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0 for any user-visible changes. -- PMM
Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
On Sat, 10 Apr 2021 at 14:53, Philippe Mathieu-Daudé wrote: > > Hi Luc, > > On 4/10/21 3:19 PM, Luc Michel wrote: > > Note that clock propagation during reset has always been a complicated > > problem. Calling clock_propagate is forbidden during the reset's enter > > phase because of the side effects it can introduce. > > Ah... Maybe this is related to the generic reset problem in QEMU :( I do wonder if we got the clock-propagation-during-reset part of this wrong -- it seemed right to me at the time but trying to use the clock API recently I did run into some unhelpful-seeming results (I forget the details now, though). > > I find your API modification a bit restrictive. I think creating a > > standalone clock can be useful, e.g. in complicated devices that may > > want to use internal "intermediate" clocks. I would not remove this > > possibility to the API users. > > Well, this is the point. I can't see a justification to have a clock > on a non-qdev object. We should be able to model complicated devices > with qdev. The obvious reason is that machine objects are not qdev devices (ie TYPE_MACHINE inherits directly from TYPE_OBJECT, not from TYPE_DEVICE), but it's a reasonable thing to say "this machine has a fixed frequency clock which it connects to the SoC". I do wonder if the right fix to that would be to make TYPE_MACHINE be a subtype of TYPE_DEVICE, though -- machines not being subtypes of device has other annoying effects, like their not having reset methods or being able to register vmstate structs. There might be some unanticipated side effects of making that change, though. thanks -- PMM
Re: [RFC PATCH-for-6.1 v2 6/6] hw/mips/loongson3_virt: Raise CPU clock to 2 GHz
Hi Huacai,
On 4/10/21 4:43 AM, Huacai Chen wrote:
> Hi, Philippe,
>
> On Fri, Apr 9, 2021 at 5:36 PM Philippe Mathieu-Daudé wrote:
>>
>> Commit cd3a53b727d ("clock: Add clock_ns_to_ticks() function")
>> removed the limitation of using clock with a frequency of 1 GHz
>> or more.
>>
>> The previous commit converted the MIPS CP0 timer to use this
>> new clock_ns_to_ticks() function. We can now use a clock of
>> 2 GHz with the Loongson3 virt board.
> Yes, we can do this, but why should we do this? I don't think this can
> bring any advantages.
IIRC this was how you sent the earlier series, then we had to reduce
the frequency to <1GHz due to the DIV#0 bug. Now it is fixed I thought
you'd get this back.
I spent time with the R4K timer because it is often used by embedded
firmwares, and a mismatch with the CPU clock makes firmware not work
well. I suppose when using Linux guests it is not a real issue, because
1/ if there is another timer (different peripheral on a system-on-soc)
Linux will use it first, 2/ Linux does some early check to adapt with
the tick rate IIRC (it doesn't assume a precise rate).
I'm fine with the current Loongson3 virt board behavior with TCG,
so let's disregard this patch.
Thanks,
Phil.
Re: [PATCH for-6.0] esp: fix setting of ESPState mig_version_id when launching QEMU with -S option
On 4/10/21 10:31 AM, Mark Cave-Ayland wrote:
> On 10/04/2021 06:56, Thomas Huth wrote:
>
>> On 07/04/2021 14.48, Mark Cave-Ayland wrote:
>>> If QEMU is launched with the -S option then the ESPState
>>> mig_version_id property
>>> is left unset due to the ordering of the VMState fields in the
>>> VMStateDescription
>>> for sysbusespscsi and pciespscsi. If the VM is migrated and restored
>>> in this
>>> stopped state, the version tests in the vmstate_esp
>>> VMStateDescription and
>>> esp_post_load() become confused causing the migration to fail.
>>>
>>> Fix the ordering problem by moving the setting of mig_version_id to a
>>> common
>>> esp_pre_save() function which is invoked first by both sysbusespscsi and
>>> pciespscsi rather than at the point where ESPState is itself
>>> serialised into the
>>> migration stream.
>>>
>>> Buglink: https://bugs.launchpad.net/qemu/+bug/1922611
>>> Fixes: 0bd005be78 ("esp: add vmstate_esp version to embedded ESPState")
>>> Signed-off-by: Mark Cave-Ayland
>>> ---
>>> hw/scsi/esp-pci.c | 1 +
>>> hw/scsi/esp.c | 7 ---
>>> include/hw/scsi/esp.h | 1 +
>>> 3 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
>>> index c3d3dab05e..9db10b1a48 100644
>>> --- a/hw/scsi/esp-pci.c
>>> +++ b/hw/scsi/esp-pci.c
>>> @@ -332,6 +332,7 @@ static const VMStateDescription
>>> vmstate_esp_pci_scsi = {
>>> .name = "pciespscsi",
>>> .version_id = 2,
>>> .minimum_version_id = 1,
>>> + .pre_save = esp_pre_save,
>>> .fields = (VMStateField[]) {
>>> VMSTATE_PCI_DEVICE(parent_obj, PCIESPState),
>>> VMSTATE_BUFFER_UNSAFE(dma_regs, PCIESPState, 0, 8 *
>>> sizeof(uint32_t)),
>>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>> index 3b9037e4f4..0037197bdb 100644
>>> --- a/hw/scsi/esp.c
>>> +++ b/hw/scsi/esp.c
>>> @@ -1089,9 +1089,10 @@ static bool esp_is_version_5(void *opaque, int
>>> version_id)
>>> return version_id == 5;
>>> }
>>> -static int esp_pre_save(void *opaque)
>>> +int esp_pre_save(void *opaque)
>>> {
>>> - ESPState *s = ESP(opaque);
>>> + ESPState *s = ESP(object_resolve_path_component(
>>> + OBJECT(opaque), "esp"));
>>> s->mig_version_id = vmstate_esp.version_id;
>>> return 0;
>>> @@ -1127,7 +1128,6 @@ const VMStateDescription vmstate_esp = {
>>> .name = "esp",
>>> .version_id = 5,
>>> .minimum_version_id = 3,
>>> - .pre_save = esp_pre_save,
>>> .post_load = esp_post_load,
>>> .fields = (VMStateField[]) {
>>> VMSTATE_BUFFER(rregs, ESPState),
>>> @@ -1317,6 +1317,7 @@ static const VMStateDescription
>>> vmstate_sysbus_esp_scsi = {
>>> .name = "sysbusespscsi",
>>> .version_id = 2,
>>> .minimum_version_id = 1,
>>> + .pre_save = esp_pre_save,
>>> .fields = (VMStateField[]) {
>>> VMSTATE_UINT8_V(esp.mig_version_id, SysBusESPState, 2),
>>> VMSTATE_STRUCT(esp, SysBusESPState, 0, vmstate_esp, ESPState),
>>> diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
>>> index 95088490aa..aada3680b7 100644
>>> --- a/include/hw/scsi/esp.h
>>> +++ b/include/hw/scsi/esp.h
>>> @@ -157,5 +157,6 @@ void esp_hard_reset(ESPState *s);
>>> uint64_t esp_reg_read(ESPState *s, uint32_t saddr);
>>> void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val);
>>> extern const VMStateDescription vmstate_esp;
>>> +int esp_pre_save(void *opaque);
>>
>> Reviewed-by: Thomas Huth
>>
>> Which tree should this patch go through? Your Sparc tree? Migration?
>> Scsi? Trivial?
>
> Previously I've considered the ESP patches to be SCSI, although the
> large ESP patchset was given approval to go via another tree which is
> why that was eventually merged via qemu-sparc.
>
> I don't mind doing the same again although I'm still waiting for the
> final nod for this and the ESP security fixes for 6.0 (see
> https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01479.html).
>
> Thoughts/other ideas?
It makes sense to have this go via the SCSI tree, but the maintainers
are pretty busy (you forgot to Cc Fam in both series). Maybe with an Ack
from them you could take the ESP patches via the SPARC tree?
Regards,
Phil.
Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
Hi Luc, On 4/10/21 3:19 PM, Luc Michel wrote: > On 08:23 Fri 09 Apr , Philippe Mathieu-Daudé wrote: >> I've been debugging some odd issue with the clocks: >> a clock created in the machine (IOW, not a qdev clock) isn't >> always resetted, thus propagating its value. >> "not always" is the odd part. In the MPS2 board, the machine >> clock is propagated. Apparently because the peripherals are >> created directly in the machine_init() handler. When moving >> them out in a SoC QOM container, the clock isn't... I'm still >> having hard time to understand what is going on. > > I think there is a misunderstanding on how the clock API works. If I > understand correctly your issue, you expect the callback of an input > clock connected to your constant "main oscillator" clock to be called on > machine reset. > > If I'm not mistaken this is not the way the API has been designed. The > callback is called only when the clock period changes. A constant clock > does not change on reset, so the callback of child clocks should not be > called. They why the children of a clock tree fed with constant clock stay with a clock of 0? Who is responsible of setting their clock to the constant value? > However devices that care about this clock value (e.g. a device that > has a clock input connected to this constant clock) should see their > standard reset callback called during reset. And they can effectively read > the clock value here and do what they need to do. > > Note that clock propagation during reset has always been a complicated > problem. Calling clock_propagate is forbidden during the reset's enter > phase because of the side effects it can introduce. Ah... Maybe this is related to the generic reset problem in QEMU :( >> Alternatively I tried to strengthen the clock API by reducing >> the clock creation in 2 cases: machine/device. This way clocks >> aren't left dangling around alone. The qdev clocks are properly >> resetted, and for the machine clocks I register a generic reset >> handler. This way is safer, but I don't think we want to keep >> adding generic reset handlers, instead we'd like to remove them. > > I find your API modification a bit restrictive. I think creating a > standalone clock can be useful, e.g. in complicated devices that may > want to use internal "intermediate" clocks. I would not remove this > possibility to the API users. Well, this is the point. I can't see a justification to have a clock on a non-qdev object. We should be able to model complicated devices with qdev. We are having various problems with the CPUs which are non-qdev devices, or recently even with the LED model...: https://www.mail-archive.com/qemu-devel@nongnu.org/msg798031.html Phil.
Re: [RFC PATCH-for-6.1 0/9] hw/clock: Strengthen machine (non-qdev) clock propagation
Hi Philippe, On 08:23 Fri 09 Apr , Philippe Mathieu-Daudé wrote: > Hi Damian, Luc, Peter. > > I've been debugging some odd issue with the clocks: > a clock created in the machine (IOW, not a qdev clock) isn't > always resetted, thus propagating its value. > "not always" is the odd part. In the MPS2 board, the machine > clock is propagated. Apparently because the peripherals are > created directly in the machine_init() handler. When moving > them out in a SoC QOM container, the clock isn't... I'm still > having hard time to understand what is going on. I think there is a misunderstanding on how the clock API works. If I understand correctly your issue, you expect the callback of an input clock connected to your constant "main oscillator" clock to be called on machine reset. If I'm not mistaken this is not the way the API has been designed. The callback is called only when the clock period changes. A constant clock does not change on reset, so the callback of child clocks should not be called. However devices that care about this clock value (e.g. a device that has a clock input connected to this constant clock) should see their standard reset callback called during reset. And they can effectively read the clock value here and do what they need to do. Note that clock propagation during reset has always been a complicated problem. Calling clock_propagate is forbidden during the reset's enter phase because of the side effects it can introduce. > > Alternatively I tried to strengthen the clock API by reducing > the clock creation in 2 cases: machine/device. This way clocks > aren't left dangling around alone. The qdev clocks are properly > resetted, and for the machine clocks I register a generic reset > handler. This way is safer, but I don't think we want to keep > adding generic reset handlers, instead we'd like to remove them. I find your API modification a bit restrictive. I think creating a standalone clock can be useful, e.g. in complicated devices that may want to use internal "intermediate" clocks. I would not remove this possibility to the API users. Thanks, -- Luc
Re: [PATCH for-6.0 2/2] block/nbd: ensure ->connection_thread is always valid
On Sat, Apr 10, 2021 at 12:56:34PM +0300, Vladimir Sementsov-Ogievskiy wrote: > 10.04.2021 11:38, Vladimir Sementsov-Ogievskiy wrote: > > 10.04.2021 11:06, Vladimir Sementsov-Ogievskiy wrote: > > > 09.04.2021 19:04, Roman Kagan wrote: > > > > Simplify lifetime management of BDRVNBDState->connection_thread by > > > > delaying the possible cleanup of it until the BDRVNBDState itself goes > > > > away. > > > > > > > > This also fixes possible use-after-free in nbd_co_establish_connection > > > > when it races with nbd_co_establish_connection_cancel. > > > > > > > > Signed-off-by: Roman Kagan > > > > > > Reviewed-by: Vladimir Sementsov-Ogievskiy > > > > > > > Ha stop, it crashes iotest 51, as nbd_clear_bdrvstate is called also from > > nbd_process_options. > > > > And this shows that we also do wrong thing when simply return from two ifs > > pre-patch (and one after-patch). Yes, after successful nbd_process options > > we should call nbd_clear_bdrvstate() on failure path. The test didn't crash at me somehow but you're right there's bug here. > And also it shows that patch is more difficult than it seems. I still think > that for 6.0 we should take a simple use-after-free-only fix, and don't care > about anything else. I agree it turned out more complex than apporpriate for 6.0. Let's proceed with the one you've posted. Thanks, Roman.
Re: Mac OS real USB device support issue
On Sat, 10 Apr 2021, Howard Spoelstra wrote: On Fri, Apr 9, 2021 at 9:37 PM Programmingkid wrote: Have you tried the proposed changes yet for libusb? Hi, Yes, I experimented with the current libusb from brew, the latest libusb code from github and a patched version based on that. I couldn't get a flash drive passed through with any of them. Running as root made no difference. My Mojave host doesn't allow unloading the kext loaded for the flash drive where Sierra allowed that, but then that should be handled by the patches. I'll link to the latest libusb and the patched version plus the patches. I guess it will not work on your host, but you might be able to persuade qemu to use them by using install_name_tool -change /usr/local/opt/libusb/lib/libusb-1.0.0.dylib @executable_path/libusb-1.0.0-latest.dylib qemu-system-ppc I'll also include the patches, libusb is easily built. https://surfdrive.surf.nl/files/index.php/s/Qs0rtTVe2qIudw4/download I think you (John and Gerd) found that detecting if a kernel driver is attached does not seem to work so it does not even get to unloading what these patches are about I think. So you first need to debug and fix libusb_kernel_driver_active() so the unloading function is called at all. I don't know how that's done on macOS but maybe querying the IO registry somehow that should have all info about devices and IOKit drivers. Regards, BALATON Zoltan
Re: [PATCH for-6.0 2/2] block/nbd: ensure ->connection_thread is always valid
10.04.2021 11:38, Vladimir Sementsov-Ogievskiy wrote: 10.04.2021 11:06, Vladimir Sementsov-Ogievskiy wrote: 09.04.2021 19:04, Roman Kagan wrote: Simplify lifetime management of BDRVNBDState->connection_thread by delaying the possible cleanup of it until the BDRVNBDState itself goes away. This also fixes possible use-after-free in nbd_co_establish_connection when it races with nbd_co_establish_connection_cancel. Signed-off-by: Roman Kagan Reviewed-by: Vladimir Sementsov-Ogievskiy Ha stop, it crashes iotest 51, as nbd_clear_bdrvstate is called also from nbd_process_options. And this shows that we also do wrong thing when simply return from two ifs pre-patch (and one after-patch). Yes, after successful nbd_process options we should call nbd_clear_bdrvstate() on failure path. And also it shows that patch is more difficult than it seems. I still think that for 6.0 we should take a simple use-after-free-only fix, and don't care about anything else. -- Best regards, Vladimir
Re: [RFC PATCH 5/5] sockets: Support multipath TCP
Daniel P. Berrangé writes:
> On Thu, Apr 08, 2021 at 08:11:59PM +0100, Dr. David Alan Gilbert (git) wrote:
>> From: "Dr. David Alan Gilbert"
>>
>> Multipath TCP allows combining multiple interfaces/routes into a single
>> socket, with very little work for the user/admin.
>>
>> It's enabled by 'mptcp' on most socket addresses:
>>
>>./qemu-system-x86_64 -nographic -incoming tcp:0:,mptcp
>>
>> Signed-off-by: Dr. David Alan Gilbert
>> ---
>> io/dns-resolver.c | 2 ++
>> qapi/sockets.json | 5 -
>> util/qemu-sockets.c | 34 ++
>> 3 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/io/dns-resolver.c b/io/dns-resolver.c
>> index 743a0efc87..b081e098bb 100644
>> --- a/io/dns-resolver.c
>> +++ b/io/dns-resolver.c
>> @@ -122,6 +122,8 @@ static int
>> qio_dns_resolver_lookup_sync_inet(QIODNSResolver *resolver,
>> .ipv4 = iaddr->ipv4,
>> .has_ipv6 = iaddr->has_ipv6,
>> .ipv6 = iaddr->ipv6,
>> +.has_mptcp = iaddr->has_mptcp,
>> +.mptcp = iaddr->mptcp,
>> };
>>
>> (*addrs)[i] = newaddr;
>> diff --git a/qapi/sockets.json b/qapi/sockets.json
>> index 2e83452797..43122a38bf 100644
>> --- a/qapi/sockets.json
>> +++ b/qapi/sockets.json
>> @@ -57,6 +57,8 @@
>> # @keep-alive: enable keep-alive when connecting to this socket. Not
>> supported
>> # for passive sockets. (Since 4.2)
>> #
>> +# @mptcp: enable multi-path TCP. (Since 6.0)
>> +#
>> # Since: 1.3
>> ##
>> { 'struct': 'InetSocketAddress',
>> @@ -66,7 +68,8 @@
>> '*to': 'uint16',
>> '*ipv4': 'bool',
>> '*ipv6': 'bool',
>> -'*keep-alive': 'bool' } }
>> +'*keep-alive': 'bool',
>> +'*mptcp': 'bool' } }
>
> I think this would need to be
>
>'*mptcp': { 'type': 'bool', 'if': 'IPPROTO_MPTCP' }
>
> so that mgmt apps can probe when it truely is supported or not for
> this build
Yes. Instance of a somewhat common anti-pattern "declare
unconditionally (this hunk), write unconditionally (previous hunk), read
conditionally (next hunk). Besides defeating introspection, it also
exposes configuration knobs that don't do anything.
>>
>> ##
>> # @UnixSocketAddress:
>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> index 8af0278f15..72527972d5 100644
>> --- a/util/qemu-sockets.c
>> +++ b/util/qemu-sockets.c
>> @@ -206,6 +206,21 @@ static int try_bind(int socket, InetSocketAddress
>> *saddr, struct addrinfo *e)
>> #endif
>> }
>>
>> +static int check_mptcp(const InetSocketAddress *saddr, struct addrinfo *ai,
>> + Error **errp)
>> +{
>> +if (saddr->has_mptcp && saddr->mptcp) {
>> +#ifdef IPPROTO_MPTCP
>> +ai->ai_protocol = IPPROTO_MPTCP;
>> +#else
>> +error_setg(errp, "MPTCP unavailable in this build");
>> +return -1;
>> +#endif
>> +}
>> +
>> +return 0;
>> +}
>> +
>> static int inet_listen_saddr(InetSocketAddress *saddr,
>> int port_offset,
>> int num,
[...]
Re: gitlab-ci check-dco test
On 4/10/21 4:58 AM, Richard Henderson wrote: > On development branches, it's not uncommon to push > temporary --fixup patches, and normally one doesn't > sign those. But then of course one get hate-mail > from the gitlab-ci job about the failing test. > > Is there a way to make it fatal on staging, but > merely a warning on other branches (a-la checkpatch)? To only run check-dco on branch /staging on any namespace: -- >8 -- diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 3480d79db3a..f0d21da57f0 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -781,9 +781,9 @@ check-dco: needs: job: amd64-centos8-container script: .gitlab-ci.d/check-dco.py - except: + only: variables: - - $CI_PROJECT_NAMESPACE == 'qemu-project' && $CI_COMMIT_BRANCH == 'master' + - $CI_COMMIT_BRANCH == 'staging' variables: GIT_DEPTH: 1000 ---
Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system
Greg Kurz writes: > Despite its simple name and common usage of "getting a pointer to > the machine" in system-mode emulation, qdev_get_machine() has some > subtilities. > > First, it can be called when running user-mode emulation : this is > because user-mode partly relies on qdev to instantiate its CPU > model. > > Second, but not least, it has a side-effect : if it cannot find an > object at "/machine" in the QOM tree, it creates a dummy "container" > object and put it there. A simple check on the type returned by > qdev_get_machine() allows user-mode to run the common qdev code, > skipping the parts that only make sense for system-mode. > > This side-effect turns out to complicate the use of qdev_get_machine() > for the system-mode case though. Most notably, qdev_get_machine() must > not be called before the machine object is added to the QOM tree by > qemu_create_machine(), otherwise the existing dummy "container" object > would cause qemu_create_machine() to fail with something like : Stupid trap. > Unexpected error in object_property_try_add() at ../../qom/object.c:1223: > qemu-system-ppc64: attempt to add duplicate property 'machine' to > object (type 'container') > Aborted (core dumped) > > This situation doesn't exist in the current code base, mostly because > of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564 > and e2fb3fbbf9c for details). I lacked the stamina to address the root problem: automatic creation of dummy containers where real ones may be needed. Is /machine the only such container? Have you reviewed the other uses of container_get()? > A new kind of breakage was spotted very recently though : > > $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help > /home/thuth/devel/qemu/include/hw/boards.h:24: > MACHINE: Object 0x5635bd53af10 is not an instance of type machine > Aborted (core dumped) > > This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly > added a new condition for qdev_get_machine() to be called too early, > breaking MACHINE(qdev_get_machine()) in generic cpu-core code this > time. > > In order to avoid further subtle breakages like this, change the > implentation of qdev_get_machine() to: > - keep the existing behaviour of creating the dummy "container" > object for the user-mode case only ; > - abort() if the machine doesn't exist yet in the QOM tree for > the system-mode case. This gives a precise hint to developpers > that calling qdev_get_machine() too early is a programming bug. In other words, we fail right away instead of planting a landmine for later. Good. The alternative would be mandating "must create /machine before first use" for all programs, not just qemu-system-FOO, but that might be more invasive. Not sure. > This is achieved with a new do_qdev_get_machine() function called container_get() is a suboptimal name for a function that creates containers, qdev_get_machine() is a suboptimal name for a function that creates /machine, and so is do_qdev_get_machine(). Observation, not demand. > from qdev_get_machine(), with different implementations for system > and user mode. > > $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help > qemu-system-ppc64: ../../hw/core/machine.c:1290: > qdev_get_machine: Assertion `machine != NULL' failed. > Aborted (core dumped) > > Reported-by: Thomas Huth > Signed-off-by: Greg Kurz
Re: [PATCH for-6.0 2/2] block/nbd: ensure ->connection_thread is always valid
10.04.2021 11:06, Vladimir Sementsov-Ogievskiy wrote: 09.04.2021 19:04, Roman Kagan wrote: Simplify lifetime management of BDRVNBDState->connection_thread by delaying the possible cleanup of it until the BDRVNBDState itself goes away. This also fixes possible use-after-free in nbd_co_establish_connection when it races with nbd_co_establish_connection_cancel. Signed-off-by: Roman Kagan Reviewed-by: Vladimir Sementsov-Ogievskiy Ha stop, it crashes iotest 51, as nbd_clear_bdrvstate is called also from nbd_process_options. And this shows that we also do wrong thing when simply return from two ifs pre-patch (and one after-patch). Yes, after successful nbd_process options we should call nbd_clear_bdrvstate() on failure path. -- Best regards, Vladimir
Re: [PATCH for-6.0] esp: fix setting of ESPState mig_version_id when launching QEMU with -S option
On 10/04/2021 06:56, Thomas Huth wrote:
On 07/04/2021 14.48, Mark Cave-Ayland wrote:
If QEMU is launched with the -S option then the ESPState mig_version_id property
is left unset due to the ordering of the VMState fields in the
VMStateDescription
for sysbusespscsi and pciespscsi. If the VM is migrated and restored in this
stopped state, the version tests in the vmstate_esp VMStateDescription and
esp_post_load() become confused causing the migration to fail.
Fix the ordering problem by moving the setting of mig_version_id to a common
esp_pre_save() function which is invoked first by both sysbusespscsi and
pciespscsi rather than at the point where ESPState is itself serialised into the
migration stream.
Buglink: https://bugs.launchpad.net/qemu/+bug/1922611
Fixes: 0bd005be78 ("esp: add vmstate_esp version to embedded ESPState")
Signed-off-by: Mark Cave-Ayland
---
hw/scsi/esp-pci.c | 1 +
hw/scsi/esp.c | 7 ---
include/hw/scsi/esp.h | 1 +
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index c3d3dab05e..9db10b1a48 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -332,6 +332,7 @@ static const VMStateDescription vmstate_esp_pci_scsi = {
.name = "pciespscsi",
.version_id = 2,
.minimum_version_id = 1,
+ .pre_save = esp_pre_save,
.fields = (VMStateField[]) {
VMSTATE_PCI_DEVICE(parent_obj, PCIESPState),
VMSTATE_BUFFER_UNSAFE(dma_regs, PCIESPState, 0, 8 * sizeof(uint32_t)),
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 3b9037e4f4..0037197bdb 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1089,9 +1089,10 @@ static bool esp_is_version_5(void *opaque, int
version_id)
return version_id == 5;
}
-static int esp_pre_save(void *opaque)
+int esp_pre_save(void *opaque)
{
- ESPState *s = ESP(opaque);
+ ESPState *s = ESP(object_resolve_path_component(
+ OBJECT(opaque), "esp"));
s->mig_version_id = vmstate_esp.version_id;
return 0;
@@ -1127,7 +1128,6 @@ const VMStateDescription vmstate_esp = {
.name = "esp",
.version_id = 5,
.minimum_version_id = 3,
- .pre_save = esp_pre_save,
.post_load = esp_post_load,
.fields = (VMStateField[]) {
VMSTATE_BUFFER(rregs, ESPState),
@@ -1317,6 +1317,7 @@ static const VMStateDescription vmstate_sysbus_esp_scsi =
{
.name = "sysbusespscsi",
.version_id = 2,
.minimum_version_id = 1,
+ .pre_save = esp_pre_save,
.fields = (VMStateField[]) {
VMSTATE_UINT8_V(esp.mig_version_id, SysBusESPState, 2),
VMSTATE_STRUCT(esp, SysBusESPState, 0, vmstate_esp, ESPState),
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index 95088490aa..aada3680b7 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -157,5 +157,6 @@ void esp_hard_reset(ESPState *s);
uint64_t esp_reg_read(ESPState *s, uint32_t saddr);
void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val);
extern const VMStateDescription vmstate_esp;
+int esp_pre_save(void *opaque);
Reviewed-by: Thomas Huth
Which tree should this patch go through? Your Sparc tree? Migration? Scsi?
Trivial?
Previously I've considered the ESP patches to be SCSI, although the large ESP
patchset was given approval to go via another tree which is why that was eventually
merged via qemu-sparc.
I don't mind doing the same again although I'm still waiting for the final nod for
this and the ESP security fixes for 6.0 (see
https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01479.html).
Thoughts/other ideas?
ATB,
Mark.
[PATCH] accel/tcg: Logs num_insns in translate_block trace events.
This makes it easier to figure out whether a particular instruction was actually translated. Signed-off-by: Nathan Ringo --- accel/tcg/trace-events| 2 +- accel/tcg/translate-all.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/accel/tcg/trace-events b/accel/tcg/trace-events index 6eefb37f5d..c227e56248 100644 --- a/accel/tcg/trace-events +++ b/accel/tcg/trace-events @@ -7,4 +7,4 @@ exec_tb_nocache(void *tb, uintptr_t pc) "tb:%p pc=0x%"PRIxPTR exec_tb_exit(void *last_tb, unsigned int flags) "tb:%p flags=0x%x" # translate-all.c -translate_block(void *tb, uintptr_t pc, const void *tb_code) "tb:%p, pc:0x%"PRIxPTR", tb_code:%p" +translate_block(void *tb, uintptr_t pc, int num_insns, const void *tb_code) "tb:%p, pc:0x%"PRIxPTR", num_insns:%d, tb_code:%p" diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index f32df8b240..d43936b9b4 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -1916,7 +1916,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu, tcg_ctx->cpu = NULL; max_insns = tb->icount; -trace_translate_block(tb, tb->pc, tb->tc.ptr); +trace_translate_block(tb, tb->pc, (int)tb->icount, tb->tc.ptr); /* generate machine code */ tb->jmp_reset_offset[0] = TB_JMP_RESET_OFFSET_INVALID; -- 2.26.3
Re: [PATCH for-6.0 2/2] block/nbd: ensure ->connection_thread is always valid
09.04.2021 19:04, Roman Kagan wrote: Simplify lifetime management of BDRVNBDState->connection_thread by delaying the possible cleanup of it until the BDRVNBDState itself goes away. This also fixes possible use-after-free in nbd_co_establish_connection when it races with nbd_co_establish_connection_cancel. Signed-off-by: Roman Kagan Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH for-6.0 1/2] block/nbd: fix channel object leak
09.04.2021 19:04, Roman Kagan wrote: nbd_free_connect_thread leaks the channel object if it hasn't been stolen. Unref it and fix the leak. Signed-off-by: Roman Kagan Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: Mac OS real USB device support issue
On Fri, Apr 9, 2021 at 9:37 PM Programmingkid wrote: > > > > > On Apr 7, 2021, at 1:28 AM, Howard Spoelstra wrote: > > > > On Wed, Apr 7, 2021 at 7:26 AM Howard Spoelstra wrote: > >> > >> On Wed, Apr 7, 2021 at 3:53 AM Programmingkid > >> wrote: > >>> > >>> > >>> > On Apr 6, 2021, at 7:18 PM, BALATON Zoltan wrote: > > On Tue, 6 Apr 2021, Programmingkid wrote: > >> On Apr 6, 2021, at 12:53 PM, BALATON Zoltan wrote: > >> On Tue, 6 Apr 2021, Programmingkid wrote: > On Apr 6, 2021, at 10:01 AM, Howard Spoelstra > wrote: > On Tue, Apr 6, 2021 at 3:44 PM Programmingkid > wrote: > > > > Hi Gerd, > > > > I was wondering if you had access to a Mac OS 10 or Mac OS 11 > > machine to test USB support. I am on Mac OS 11.1 and cannot make > > USB devices work with any of my guests. So far these are the guests > > I have tested with: > > > > - Windows 7 > > - Mac OS 9.2 > > - Windows 2000 > > > > I have tried using USB flash drives, USB sound cards, and an USB > > headset. They all show up under 'info usb', but cannot be used in > > the guest. My setup does use a USB-C hub so I'm not sure if this is > > a bug with QEMU or an issue with the hub. Would you have any > > information on this issue? > > Hi John, > > As far as the Mac OS 9.2 guest is concerned on a mac OS host, it does > not support USB 2.0. I was successful only in passing through a USB > flash drive that was forced into USB 1.1 mode by connecting it to a > real USB 1.1 hub and unloading the kext it used. > > Best, > Howard > >>> > >>> Hi Howard, I was actually thinking about CC'ing you for this email. > >>> Glad you found it. Unloading kext files does not sound pleasant. > >>> Maybe there is some better way of doing it. > >> > >> In any case, until you make sure nothing tries to drive the device on > >> the host, passing it to a guest likely will fail because then two > >> drivers from two OSes would try to access it simultaneously which > >> likely creates a mess as the device and drivers don't expect this. So > >> you can't just pass a device through that the host has recognised and > >> is driving without somehow getting the host to leave it alone first > >> before you can pass it through. Unloading the driver is one way to do > >> that (although it probably breaks all other similar devices too). > >> Maybe there's another way to unbind a device from the host such as > >> ejecting it first but then I'm not sure if the low level USB needed > >> for accessing the device still works after that or it's completely > >> forgotten. There's probably a doc somewhere that describes how it > >> works and how can you plug a device without also getting higher level > >> drivers to load or if there's no official ways for that then you'll > >> need to do some configuration on the host t > o avoid it grabbing devices that you want to pass through. On Linux you > can add an udev rule to ignore the device (maybe also adding > TAG+="uaccess" to allow console users to use it without needing root > access) but not sure how USB works on macOS. > >> > >> Regards, > >> BALATON Zoltan > > > > Being able to dissociate a real USB device from its Mac OS driver would > > be very useful in this situation. IOKit might be one place to look for > > such a feature. The Mach kernel documentation is another place that > > might have what we want. > > Those might be a good place to start. IOKit provides the drivers and > also the io registry which is probably where you can get if a driver is > bound to a device and which one is it. How to dissociate the driver from > the device though I don't know. > >>> > >>> https://developer.apple.com/library/archive/documentation/DeviceDrivers/Conceptual/IOKitFundamentals/DeviceRemoval/DeviceRemoval.html > >>> According to this article a driver has a stop() and detach() method that > >>> is called by the IOKit to remove a device. I'm thinking QEMU can be the > >>> one that calls these methods for a certain device. > >>> > > > I have one theory. What if we introduce a middleman. A pseudo-USB > > device that the guest operating system could apply its configuration > > data to and will also talk directly with to the real USB device. > > So this: > > > > USB device <-> Host <-> QEMU USB middleman <-> Guest > > Isn't this middleman the QEMU usb-host device that we already have? > >>> > >>> It could be. I need to research this issue some more. > >>> > > > This could make USB 2.0 and 3.0 flash drives compatible with an older > > o
Re: [PATCH 15/24] aspeed/smc: Add extra controls to request DMA
On 4/9/21 8:54 AM, Joel Stanley wrote:
> On Wed, 7 Apr 2021 at 17:17, Cédric Le Goater wrote:
>>
>> The AST2600 SPI controllers have a set of bits to request/grant DMA
>> access. Add a new SMC feature for these controllers and use it to
>> check access to the DMA registers.
>
> Ah this is why you added the features mask. Makes sense.
Yes. It's a bit redundant with the dma_ctrl() handler but it looks cleaner.
>
> Reviewed-by: Joel Stanley
Thanks,
C.
>>
>> Cc: Chin-Ting Kuo
>> Signed-off-by: Cédric Le Goater
>> ---
>> include/hw/ssi/aspeed_smc.h | 1 +
>> hw/ssi/aspeed_smc.c | 74 +
>> 2 files changed, 68 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
>> index 07879fd1c4a7..cdaf165300b6 100644
>> --- a/include/hw/ssi/aspeed_smc.h
>> +++ b/include/hw/ssi/aspeed_smc.h
>> @@ -55,6 +55,7 @@ typedef struct AspeedSMCController {
>> const AspeedSegments *seg);
>> void (*reg_to_segment)(const struct AspeedSMCState *s, uint32_t reg,
>> AspeedSegments *seg);
>> +void (*dma_ctrl)(struct AspeedSMCState *s, uint32_t value);
>> } AspeedSMCController;
>>
>> typedef struct AspeedSMCFlash {
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index 4521bbd4864e..189b35637c77 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -127,6 +127,8 @@
>>
>> /* DMA Control/Status Register */
>> #define R_DMA_CTRL(0x80 / 4)
>> +#define DMA_CTRL_REQUEST (1 << 31)
>> +#define DMA_CTRL_GRANT(1 << 30)
>> #define DMA_CTRL_DELAY_MASK 0xf
>> #define DMA_CTRL_DELAY_SHIFT 8
>> #define DMA_CTRL_FREQ_MASK0xf
>> @@ -228,6 +230,7 @@ static uint32_t aspeed_smc_segment_to_reg(const
>> AspeedSMCState *s,
>>const AspeedSegments *seg);
>> static void aspeed_smc_reg_to_segment(const AspeedSMCState *s, uint32_t reg,
>>AspeedSegments *seg);
>> +static void aspeed_smc_dma_ctrl(AspeedSMCState *s, uint32_t value);
>>
>> /*
>> * AST2600 definitions
>> @@ -257,7 +260,10 @@ static uint32_t aspeed_2600_smc_segment_to_reg(const
>> AspeedSMCState *s,
>> const AspeedSegments *seg);
>> static void aspeed_2600_smc_reg_to_segment(const AspeedSMCState *s,
>> uint32_t reg, AspeedSegments
>> *seg);
>> +static void aspeed_2600_smc_dma_ctrl(AspeedSMCState *s, uint32_t value);
>> +
>> #define ASPEED_SMC_FEATURE_DMA 0x1
>> +#define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
>>
>> static inline bool aspeed_smc_has_dma(const AspeedSMCState *s)
>> {
>> @@ -281,6 +287,7 @@ static const AspeedSMCController controllers[] = {
>> .nregs = ASPEED_SMC_R_SMC_MAX,
>> .segment_to_reg= aspeed_smc_segment_to_reg,
>> .reg_to_segment= aspeed_smc_reg_to_segment,
>> +.dma_ctrl = aspeed_smc_dma_ctrl,
>> }, {
>> .name = "aspeed.fmc-ast2400",
>> .r_conf= R_CONF,
>> @@ -299,6 +306,7 @@ static const AspeedSMCController controllers[] = {
>> .nregs = ASPEED_SMC_R_MAX,
>> .segment_to_reg= aspeed_smc_segment_to_reg,
>> .reg_to_segment= aspeed_smc_reg_to_segment,
>> +.dma_ctrl = aspeed_smc_dma_ctrl,
>> }, {
>> .name = "aspeed.spi1-ast2400",
>> .r_conf= R_SPI_CONF,
>> @@ -315,6 +323,7 @@ static const AspeedSMCController controllers[] = {
>> .nregs = ASPEED_SMC_R_SPI_MAX,
>> .segment_to_reg= aspeed_smc_segment_to_reg,
>> .reg_to_segment= aspeed_smc_reg_to_segment,
>> +.dma_ctrl = aspeed_smc_dma_ctrl,
>> }, {
>> .name = "aspeed.fmc-ast2500",
>> .r_conf= R_CONF,
>> @@ -333,6 +342,7 @@ static const AspeedSMCController controllers[] = {
>> .nregs = ASPEED_SMC_R_MAX,
>> .segment_to_reg= aspeed_smc_segment_to_reg,
>> .reg_to_segment= aspeed_smc_reg_to_segment,
>> +.dma_ctrl = aspeed_smc_dma_ctrl,
>> }, {
>> .name = "aspeed.spi1-ast2500",
>> .r_conf= R_CONF,
>> @@ -349,6 +359,7 @@ static const AspeedSMCController controllers[] = {
>> .nregs = ASPEED_SMC_R_MAX,
>> .segment_to_reg= aspeed_smc_segment_to_reg,
>> .reg_to_segment= aspeed_smc_reg_to_segment,
>> +.dma_ctrl = aspeed_smc_dma_ctrl,
>> }, {
>> .name = "aspeed.spi2-ast2500",
>> .r_conf= R_CONF,
>> @@ -365,6 +376,7 @@ static const AspeedSMCController controllers[] = {
>> .nregs = ASPEED_SMC_R_MAX,
>> .segment_to_reg= aspeed_smc_segment_to_re
