Re: [Qemu-devel] [PATCH 2/7] tcg: Optimize add2 + sub2
On Thu, Sep 27, 2012 at 5:19 PM, Richard Henderson wrote:
> We can't do complete constant folding because we lack "mov2",
> or the ability to insert opcodes in the stream. But we can
> at least canonicalize add2 operand ordering and simplify
> add2 to add when the lowpart adds a constant 0.
Couldn't we introduce add2_part1 and add2_part2, the latter being nop
for architectures that don't need it?
>
> Signed-off-by: Richard Henderson
> ---
> tcg/optimize.c | 31 +++
> 1 file changed, 31 insertions(+)
>
> diff --git a/tcg/optimize.c b/tcg/optimize.c
> index 55f2a24..004c336 100644
> --- a/tcg/optimize.c
> +++ b/tcg/optimize.c
> @@ -470,6 +470,11 @@ static TCGArg *tcg_constant_folding(TCGContext *s,
> uint16_t *tcg_opc_ptr,
> if (swap_commutative(args[0], &args[4], &args[3])) {
> args[5] = tcg_invert_cond(args[5]);
> }
> +break;
> +case INDEX_op_add2_i32:
> +swap_commutative(args[0], &args[2], &args[4]);
> +swap_commutative(args[1], &args[3], &args[5]);
> +break;
> default:
> break;
> }
> @@ -522,6 +527,32 @@ static TCGArg *tcg_constant_folding(TCGContext *s,
> uint16_t *tcg_opc_ptr,
> continue;
> }
> break;
> +case INDEX_op_add2_i32:
> +case INDEX_op_sub2_i32:
> +/* Simplify op rl, rh, al, ah, 0, bh => op rh, ah, bh.
> + The zero implies there will be no carry into the high part.
> + But only when rl == al, since we can't insert the extra move
> + that would be required. */
> +if (temps[args[4]].state == TCG_TEMP_CONST
> +&& temps[args[4]].val == 0
> +&& temps_are_copies(args[0], args[2])) {
> +if (temps[args[5]].state == TCG_TEMP_CONST
> +&& temps[args[5]].val == 0
> +&& temps_are_copies(args[1], args[3])) {
> +gen_opc_buf[op_index] = INDEX_op_nop;
> +} else {
> +gen_opc_buf[op_index] = (op == INDEX_op_add2_i32
> + ? INDEX_op_add_i32
> + : INDEX_op_sub_i32);
> +args[0] = args[1];
> +args[1] = args[3];
> +args[2] = args[5];
> +gen_args += 3;
> +}
> +args += 6;
> +continue;
> +}
> +break;
> default:
> break;
> }
> --
> 1.7.11.4
>
>
Re: [Qemu-devel] instruction execution permission
On Sun, Sep 30, 2012 at 4:06 AM, Xin Tong wrote: > In QEMU x86, is a basicblock is translated with CPL of 0, can the > translation block be executed in CPL of 3 ? TB flags are defined by cpu_get_tb_cpu_state(), you can find the x86 version in target-i386/cpu.h. > > Xin >
Re: [Qemu-devel] [big lock] Discussion about the convention of device's DMA each other after breaking down biglock
On 09/29/2012 11:20 AM, liu ping fan wrote: > > Do we have iommus in qemu now, We do, but they're hacked into the scsi layer, see hw/sun4m_iommu.c. I don't know if it's a standalone iommu on real hardware or whether it is part of the HBA. > since there are no separate phys_maps > for real address and dev's virt address, and I think the iommu is only > needed by host, not guest, so need not emulated by qemu. Eventually we will emulate iommus for x86 too, so we need to consider them. > If no, we > can just reject the nested DMA, and the c_p_m_rw() can only be nested > once, so if introduce a wrapper for c_p_m_rw(), we can avoid > recursive big lock, right? Don't we need that for other reasons? If not, we can drop it for now. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [big lock] Discussion about the convention of device's DMA each other after breaking down biglock
On Sun, Sep 30, 2012 at 4:13 PM, Avi Kivity wrote: > On 09/29/2012 11:20 AM, liu ping fan wrote: >> >> Do we have iommus in qemu now, > > We do, but they're hacked into the scsi layer, see hw/sun4m_iommu.c. I > don't know if it's a standalone iommu on real hardware or whether it is > part of the HBA. > >> since there are no separate phys_maps >> for real address and dev's virt address, and I think the iommu is only >> needed by host, not guest, so need not emulated by qemu. > > Eventually we will emulate iommus for x86 too, so we need to consider them. > For nested guest? The address translation is like nested mmu? >> If no, we >> can just reject the nested DMA, and the c_p_m_rw() can only be nested >> once, so if introduce a wrapper for c_p_m_rw(), we can avoid >> recursive big lock, right? > > Don't we need that for other reasons? If not, we can drop it for now. > Yes, there is another reason is about the unplug process as you suggest, DeviceState::uninit() can call del timer with nested biglock. Or as alternative, we del timer when unplug and check the valid of timer when mmio-dispatch in e1000. Regards, pingfan > -- > error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [big lock] Discussion about the convention of device's DMA each other after breaking down biglock
On Sun, Sep 30, 2012 at 8:13 AM, Avi Kivity wrote: > On 09/29/2012 11:20 AM, liu ping fan wrote: >> >> Do we have iommus in qemu now, > > We do, but they're hacked into the scsi layer, see hw/sun4m_iommu.c. I > don't know if it's a standalone iommu on real hardware or whether it is > part of the HBA. It's standalone or even part of CPU (some uniprocessor systems). IOMMU sits between memory and SBus, so all SBus devices (currently only ESP and Lance) use it. > >> since there are no separate phys_maps >> for real address and dev's virt address, and I think the iommu is only >> needed by host, not guest, so need not emulated by qemu. > > Eventually we will emulate iommus for x86 too, so we need to consider them. > >> If no, we >> can just reject the nested DMA, and the c_p_m_rw() can only be nested >> once, so if introduce a wrapper for c_p_m_rw(), we can avoid >> recursive big lock, right? > > Don't we need that for other reasons? If not, we can drop it for now. I don't think nested DMA is ever needed, it would be pretty obscure feature and it would need a pretty heavy implementation (recursion) in a real HW IOMMU. In theory the translated address may map to MMIO but that's different. > > -- > error compiling committee.c: too many arguments to function >
Re: [Qemu-devel] [big lock] Discussion about the convention of device's DMA each other after breaking down biglock
On 09/30/2012 01:04 PM, Blue Swirl wrote: > On Sun, Sep 30, 2012 at 8:13 AM, Avi Kivity wrote: >> On 09/29/2012 11:20 AM, liu ping fan wrote: >>> >>> Do we have iommus in qemu now, >> >> We do, but they're hacked into the scsi layer, see hw/sun4m_iommu.c. I >> don't know if it's a standalone iommu on real hardware or whether it is >> part of the HBA. > > It's standalone or even part of CPU (some uniprocessor systems). IOMMU > sits between memory and SBus, so all SBus devices (currently only ESP > and Lance) use it. So, the current modelling is incorrect? I'd like to fix it as a way of getting proper iommu modelling, but I don't know how it should be done, or how to test it. > >> >>> since there are no separate phys_maps >>> for real address and dev's virt address, and I think the iommu is only >>> needed by host, not guest, so need not emulated by qemu. >> >> Eventually we will emulate iommus for x86 too, so we need to consider them. >> >>> If no, we >>> can just reject the nested DMA, and the c_p_m_rw() can only be nested >>> once, so if introduce a wrapper for c_p_m_rw(), we can avoid >>> recursive big lock, right? >> >> Don't we need that for other reasons? If not, we can drop it for now. > > I don't think nested DMA is ever needed, it would be pretty obscure > feature and it would need a pretty heavy implementation (recursion) in > a real HW IOMMU. In theory the translated address may map to MMIO but > that's different. Sure. But if we can get a model that works for everything at no extra cost, then why not? btw, the model of 'either we take the big lock recusrsively, or we drop the local lock before issuing dma' seems to cover nested iommus with any mix of big locks and little locks. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [big lock] Discussion about the convention of device's DMA each other after breaking down biglock
On 09/30/2012 10:48 AM, liu ping fan wrote: > On Sun, Sep 30, 2012 at 4:13 PM, Avi Kivity wrote: >> On 09/29/2012 11:20 AM, liu ping fan wrote: >>> >>> Do we have iommus in qemu now, >> >> We do, but they're hacked into the scsi layer, see hw/sun4m_iommu.c. I >> don't know if it's a standalone iommu on real hardware or whether it is >> part of the HBA. >> >>> since there are no separate phys_maps >>> for real address and dev's virt address, and I think the iommu is only >>> needed by host, not guest, so need not emulated by qemu. >> >> Eventually we will emulate iommus for x86 too, so we need to consider them. >> > For nested guest? The address translation is like nested mmu? Yes. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [big lock] Discussion about the convention of device's DMA each other after breaking down biglock
On Sun, Sep 30, 2012 at 11:17 AM, Avi Kivity wrote: > On 09/30/2012 01:04 PM, Blue Swirl wrote: >> On Sun, Sep 30, 2012 at 8:13 AM, Avi Kivity wrote: >>> On 09/29/2012 11:20 AM, liu ping fan wrote: Do we have iommus in qemu now, >>> >>> We do, but they're hacked into the scsi layer, see hw/sun4m_iommu.c. I >>> don't know if it's a standalone iommu on real hardware or whether it is >>> part of the HBA. >> >> It's standalone or even part of CPU (some uniprocessor systems). IOMMU >> sits between memory and SBus, so all SBus devices (currently only ESP >> and Lance) use it. > > So, the current modelling is incorrect? I'd like to fix it as a way of > getting proper iommu modelling, but I don't know how it should be done, > or how to test it. The model (opaque IOMMU handle + ESP/Lance specific accessors) could be improved much, it predates all IOMMU discussions. Just grab any ISO (preferably a sparc32 one), try booting. If it fails, there's a problem because IOMMU is enabled by OpenBIOS. > >> >>> since there are no separate phys_maps for real address and dev's virt address, and I think the iommu is only needed by host, not guest, so need not emulated by qemu. >>> >>> Eventually we will emulate iommus for x86 too, so we need to consider them. >>> If no, we can just reject the nested DMA, and the c_p_m_rw() can only be nested once, so if introduce a wrapper for c_p_m_rw(), we can avoid recursive big lock, right? >>> >>> Don't we need that for other reasons? If not, we can drop it for now. >> >> I don't think nested DMA is ever needed, it would be pretty obscure >> feature and it would need a pretty heavy implementation (recursion) in >> a real HW IOMMU. In theory the translated address may map to MMIO but >> that's different. > > Sure. But if we can get a model that works for everything at no extra > cost, then why not? That's OK, I'm not opposing it. > > btw, the model of 'either we take the big lock recusrsively, or we drop > the local lock before issuing dma' seems to cover nested iommus with any > mix of big locks and little locks. > > -- > error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH 18/22] target-i386: parse cpu_model string into set of stringified properties
On Wed, Sep 26, 2012 at 8:32 PM, Igor Mammedov wrote:
> cpu_model string does represent features in following format:
> ([+-]feat)|(feat=foo)|(feat)
> which makes it impossible directly use property infrastructure
> to set features on CPU.
> This patch introduces parser that splits CPU name from cpu_model and
> converts legacy features string into canonized set of strings that
> is compatible with property manipulation infrastructure.
>
> PS:
> * later it could be used as a hook to convert legacy command line
> features to global properties. Then marked as deprecated and
> removed with -cpu option in the future.
>
> Signed-off-by: Igor Mammedov
> ---
> v2:
> * compiler complains that it's unused function but I guess it is
> easier for review this way, for pull req I'll squash it into next
> patch
> * fix spelling error
> * initialize sptr, due to a CentOS6 compiler warning, that
> breakes build when -Werror is set. suggested-by: Don Slutz
> ---
> target-i386/cpu.c | 52
> 1 files changed, 52 insertions(+), 0 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index b7a9cd6..afa12ca 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1342,6 +1342,58 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t
> *def, Error **errp)
> }
> }
>
> +/* convert legacy cpumodel string to string cpu_name and
> + * a uniform set of custom features that will be applied to CPU
> + * using object_property_parse()
> + */
> +static void compat_normalize_cpu_model(const char *cpu_model, char
> **cpu_name,
> +QDict **features, Error **errp)
> +{
> +
> +char *s = g_strdup(cpu_model);
> +char *featurestr, *sptr = NULL;
> +
> +*cpu_name = strtok_r(s, ",", &sptr);
This would break build since strtok_r() isn't available on Mingw, it's
not Posix either. How about g_strsplit()?
> +*features = qdict_new();
> +
> +featurestr = strtok_r(NULL, ",", &sptr);
> +while (featurestr) {
> +char *val;
> +if (featurestr[0] == '+') {
> +/*
> + * preseve legacy behaviour, if feature was disabled once
> + * do not allow to enable it again
> + */
> +if (!qdict_haskey(*features, featurestr + 1)) {
> +qdict_put(*features, featurestr + 1, qstring_from_str("on"));
> +}
> +} else if (featurestr[0] == '-') {
> +qdict_put(*features, featurestr + 1, qstring_from_str("off"));
> +} else {
> +val = strchr(featurestr, '=');
> +if (val) {
> +*val = 0; val++;
> +if (!strcmp(featurestr, "vendor")) {
> +qdict_put(*features, "vendor-override",
> + qstring_from_str("on"));
> +qdict_put(*features, featurestr, qstring_from_str(val));
> +} else if (!strcmp(featurestr, "tsc_freq")) {
> +qdict_put(*features, "tsc-frequency",
> + qstring_from_str(val));
> +} else {
> +qdict_put(*features, featurestr, qstring_from_str(val));
> +}
> +} else {
> +qdict_put(*features, featurestr, qstring_from_str("on"));
> +}
> +}
> +
> +featurestr = strtok_r(NULL, ",", &sptr);
> +}
> +
> +return;
> +}
> +
> static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> const char *cpu_model, Error **errp)
> {
> --
> 1.7.1
>
Re: [Qemu-devel] [PATCH 0/5] i386: cpu: remove duplicate feature names
On Thu, Sep 6, 2012 at 8:05 PM, Eduardo Habkost wrote: > The problem: > > - Some features are report at the same time on both CPUID[1].EDX and >CPUID[8000_0001].EDX on AMD CPUs (e.g. fpu, tsc, msr, pae, mmx). > - "-cpu ,+feature" should enable the bit only on CPUID[1] if >it's not an AMD CPU, but it should enable the bit on both CPUID[1] and >CPUID[8000_0001] if it's an AMD CPU. > - The same should happen when implementing CPU properties: setting the >property that enables a feature should set the duplicate > CPUID[8000_0001].EDX >bit only if CPU vendor is AMD. > > Reference: http://article.gmane.org/gmane.comp.emulators.qemu/166024 > > The solution implemented by this series is: > - On the CPU model table and while parsing CPU options/properties, set the > bit >only on CPUID[1] (the x86_def_t.features field). > - When finishing initialization of the CPU cpuid fields, duplicate those >feature bits on cpuid_ext2_features if and only if the CPU vendor is AMD. > > This series depends on the "x86 CPU patches that didn't get into 1.2" series: > http://article.gmane.org/gmane.comp.emulators.qemu/168633 > Message-Id: <1346877673-9136-1-git-send-email-ehabk...@redhat.com> Thanks, applied all. > > > Eduardo Habkost (5): > i386: kvm: bit 10 of CPUID[8000_0001].EDX is reserved > i386: kvm: use a #define for the set of alias feature bits > i386: cpu: replace EXT2_FEATURE_MASK with CPUID_EXT2_AMD_ALIASES > i386: cpu: eliminate duplicate feature names > i386: -cpu help: remove reference to specific CPUID leaves/registers > > target-i386/cpu.c | 59 > +++ > target-i386/cpu.h | 12 +++ > target-i386/kvm.c | 2 +- > 3 files changed, 51 insertions(+), 22 deletions(-) > > -- > 1.7.11.4 > >
Re: [Qemu-devel] [PATCH] esp: On qemu-system-sparc's esp, TC is not set properly
On Tue, Sep 25, 2012 at 6:15 PM, Ryo ONODERA wrote:
> Hi,
>
> From: Ryo ONODERA , Date: Wed, 26 Sep 2012 02:44:35 +0900
>
>> I does not understand hw/esp.c fully.
>> This patch supresses esp/TC related errors on NetBSD/sparc 5.1.2 or 6.0_RC2.
>>
>> Please review the patch.
>>
>> This is patch for https://bugs.launchpad.net/qemu/+bug/1055090 .
>>
>> Signed-off-by: Ryo ONODERA
>> ---
>> hw/esp.c | 8
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/esp.c b/hw/esp.c
>> index 84a4e74..cd14ed1 100644
>> --- a/hw/esp.c
>> +++ b/hw/esp.c
>> @@ -91,6 +91,10 @@ static uint32_t get_cmd(ESPState *s, uint8_t *buf)
>> dmalen |= s->rregs[ESP_TCMID] << 8;
>> dmalen |= s->rregs[ESP_TCHI] << 16;
>> s->dma_memory_read(s->dma_opaque, buf, dmalen);
>> +s->rregs[ESP_RSTAT] |= STAT_TC;
>> +s->rregs[ESP_TCLO] = 0;
>> +s->rregs[ESP_TCMID] = 0;
>> +s->rregs[ESP_TCHI] = 0;
>> } else {
>> dmalen = s->ti_size;
>> memcpy(buf, s->ti_buf, dmalen);
>> @@ -242,6 +246,10 @@ static void esp_do_dma(ESPState *s)
>> if (s->do_cmd) {
>> trace_esp_do_dma(s->cmdlen, len);
>> s->dma_memory_read(s->dma_opaque, &s->cmdbuf[s->cmdlen], len);
>> +s->rregs[ESP_RSTAT] |= STAT_TC;
>> +s->rregs[ESP_TCLO] = 0;
>> +s->rregs[ESP_TCMID] = 0;
>> +s->rregs[ESP_TCHI] = 0;
>> s->ti_size = 0;
>> s->cmdlen = 0;
>> s->do_cmd = 0;
>
> Sadly I have gotten same errors on NetBSD/sparc just after the report.
>
> esp0: !TC on DATA XFER [intr 10, stat 83, step 0] prevphase 2, resid 0
>
> Could qemu experts investigate this problem?
The error seems to be harmless, I was able to install NetBSD/sparc
5.1.2 and boot the installed system.
But the patch looks OK too except maybe RSTAT may need to be set
instead of ORed, possibly with other bits like other cases.
>
> Thank you.
>
> --
> Ryo ONODERA // ryo...@yk.rim.or.jp
> PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
>
>
>
[Qemu-devel] pwrite64 error because of argument position
This error may be a PPC specific problem, but I don't have another
environment where I can test it (i386 doesn't seem to use pwrite64),
so I ask for a bit of help/check.
I am in a 32bit linux testing the qemu-ppc.
My test program:
// ---
#include
#include
#include
#include
#include
int main (void) {
int fd, len;
char* asd = "This is a random test";
char asd2[20];
fd = open ("./test.pwrite", O_RDWR | O_CREAT);
printf ( "fd: %d\n", fd);
pwrite ( fd, asd, 15, 0 );
pwrite ( fd, asd+5, 6, 10);
pwrite ( fd, asd, 4, 30);
len = pread ( fd, asd2, 5, 5);
asd2[len]='\0';
printf ( "Read %d bytes: %s", len, asd2);
// This is to force two int arguments for 64bit
//len = pread ( fd, asd2, -1, -1);
close(fd);
return 0;
}
// ---
Then I do
$ powerpc-linux-gnu-gcc -g --static -o pwrite-alien pwrite-test.c
and when I launch a qemu-ppc to test, it should be (on the file)
This is a is a rThis
instead I get:
This rs a randorThis
and if I print some debug information inside pwrite64 and pread64 I see:
syscall: pwrite arg_i: 3 268909324 15 0 0 0 0 0
syscall: pwrite arg_i: 3 268909329 6 0 0 10 0 0
syscall: pwrite arg_i: 3 268909324 4 0 0 30 0 0
syscall: pread arg_i: 3 1082133156 5 0 0 5 0 0
(those are arg1, arg2, arg3, arg4, arg5, arg6 and the unused arg7 and arg8)
As can be seen, arg4 is not used, and the line
ret = get_errno(pwrite64(arg1, p, arg3, target_offset64(arg4, arg5)));
should be, in my case
ret = get_errno(pwrite64(arg1, p, arg3, target_offset64(arg5, arg6)));
With this changes, the qemu "Works For Me (TM)".
So, anybody can confirm it or (if it is indeed my problem) can give me
some pointers? I will not post this as a patch until I understand the
problem... and first step is making sure that it really is a qemu
problem. And not my toolchain or something random.
Re: [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification
On Thu, Sep 20, 2012 at 09:46:41AM -0300, Marcelo Tosatti wrote:
> On Thu, Sep 20, 2012 at 01:55:20PM +0530, Amit Shah wrote:
> > Commit f349c12c0434e29c79ecde89029320c4002f7253 added the guest stop
> > notification, but it did it in a way that the stop notification would
> > never reach the kernel. The kvm_vm_state_changed() function gets a
> > value of 0 for the 'running' parameter when the VM is stopped, making
> > all the code added previously dead code.
> >
> > This patch reworks the code so that it's called when 'running' is 0,
> > which indicates the VM was stopped.
> >
> > CC: Eric B Munson
> > CC: Raghavendra K T
> > CC: Andreas Färber
> > CC: Marcelo Tosatti
> > CC: Paolo Bonzini
> > CC: Laszlo Ersek
> > Signed-off-by: Amit Shah
> > ---
> > hw/kvm/clock.c | 21 +++--
> > 1 files changed, 11 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/kvm/clock.c b/hw/kvm/clock.c
> > index 824b978..f3427eb 100644
> > --- a/hw/kvm/clock.c
> > +++ b/hw/kvm/clock.c
> > @@ -71,18 +71,19 @@ static void kvmclock_vm_state_change(void *opaque, int
> > running,
> >
> > if (running) {
> > s->clock_valid = false;
> > +return;
> > +}
> >
> > -if (!cap_clock_ctrl) {
> > -return;
> > -}
> > -for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
> > -ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
> > -if (ret) {
> > -if (ret != -EINVAL) {
> > -fprintf(stderr, "%s: %s\n", __func__, strerror(-ret));
> > -}
> > -return;
> > +if (!cap_clock_ctrl) {
> > +return;
> > +}
> > +for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
> > +ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
> > +if (ret) {
> > +if (ret != -EINVAL) {
> > +fprintf(stderr, "%s: %s\n", __func__, strerror(-ret));
> > }
> > +return;
> > }
> > }
> > }
> > --
> > 1.7.7.6
>
> ACK
>
> Avi, please merge through uq/master.
NACK, guest should be notified when the VM is starting, not
when stopping.
Re: [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification
- Original Message -
> On Thu, Sep 20, 2012 at 09:46:41AM -0300, Marcelo Tosatti wrote:
> > On Thu, Sep 20, 2012 at 01:55:20PM +0530, Amit Shah wrote:
> > > Commit f349c12c0434e29c79ecde89029320c4002f7253 added the guest
> > > stop
In commitlog of f349c12c0434e29c79ecde89029320c4002f7253:
## This patch uses the qemu Notifier system to tell the guest it _is about to
be_ stopped
> > > notification, but it did it in a way that the stop notification
> > > would
> > > never reach the kernel. The kvm_vm_state_changed() function gets
> > > a
> > > value of 0 for the 'running' parameter when the VM is stopped,
> > > making
> > > all the code added previously dead code.
> > >
> > > This patch reworks the code so that it's called when 'running' is
> > > 0,
> > > which indicates the VM was stopped.
Amit, did you touch any real issue? guest gets call trace with current code?
which kind of context?
Someone told me he got call trace when shutdown guest by 'init 0', I didn't
verify this issue.
> > > CC: Eric B Munson
> > > CC: Raghavendra K T
> > > CC: Andreas Färber
> > > CC: Marcelo Tosatti
> > > CC: Paolo Bonzini
> > > CC: Laszlo Ersek
> > > Signed-off-by: Amit Shah
> > > ---
> > > hw/kvm/clock.c | 21 +++--
> > > 1 files changed, 11 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/hw/kvm/clock.c b/hw/kvm/clock.c
> > > index 824b978..f3427eb 100644
> > > --- a/hw/kvm/clock.c
> > > +++ b/hw/kvm/clock.c
> > > @@ -71,18 +71,19 @@ static void kvmclock_vm_state_change(void
> > > *opaque, int running,
I found this function is only called when resume vm
(here running is 1, it means vm is already resumed?
we don't call that ioctl _before_ resume).
kvmclock_vm_state_change() is not called when I stop vm
through qemu monitor command.
> > >
> > > if (running) {
> > > s->clock_valid = false;
> > > +return;
> > > +}
> > >
> > > -if (!cap_clock_ctrl) {
> > > -return;
> > > -}
> > > -for (penv = first_cpu; penv != NULL; penv =
> > > penv->next_cpu) {
> > > -ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
> > > -if (ret) {
> > > -if (ret != -EINVAL) {
> > > -fprintf(stderr, "%s: %s\n", __func__,
> > > strerror(-ret));
> > > -}
> > > -return;
> > > +if (!cap_clock_ctrl) {
> > > +return;
> > > +}
> > > +for (penv = first_cpu; penv != NULL; penv = penv->next_cpu)
> > > {
> > > +ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
> > > +if (ret) {
> > > +if (ret != -EINVAL) {
> > > +fprintf(stderr, "%s: %s\n", __func__,
> > > strerror(-ret));
> > > }
> > > +return;
> > > }
> > > }
> > > }
> > > --
> > > 1.7.7.6
> >
> > ACK
> >
> > Avi, please merge through uq/master.
>
> NACK, guest should be notified when the VM is starting, not
> when stopping.
# from api.txt
ioctl (KVM_CAP_KVMCLOCK_CTRL) can be called any time _after_ pausing
the vcpu, but _before_ it is resumed.
Thanks, Amos
[Qemu-devel] List of Virtualized IO devices - QEMU
Where can I find the list of IO devices virtualized for QEMU ? -- Regards, Krishna Kumaar N.I.
[Qemu-devel] [PATCH v6 2/8] vl: fix -hdachs/-hda argument order parsing issues
Without this patch, the -hdachs argument had to occur either
BEFORE the corresponding "-hda" option, or AFTER the plain
disk image name (if neither -hda nor -drive is used). Otherwise
it would effectively be ignored.
Option -hdachs still has no effect on -drive, but that seems best.
Signed-off-by: Matthew Ogilvie
---
vl.c | 39 ++-
1 file changed, 18 insertions(+), 21 deletions(-)
diff --git a/vl.c b/vl.c
index 8d305ca..e1d2f7e 100644
--- a/vl.c
+++ b/vl.c
@@ -2362,8 +2362,9 @@ int main(int argc, char **argv, char **envp)
char boot_devices[33] = "cad"; /* default to HD->floppy->CD-ROM */
DisplayState *ds;
DisplayChangeListener *dcl;
-int cyls, heads, secs, translation;
-QemuOpts *hda_opts = NULL, *opts, *machine_opts;
+char hdachs_params[512]; /* save -hdachs to apply to later -hda */
+QemuOpts *hda_opts = NULL; /* save -hda to be modified by later -hdachs */
+QemuOpts *opts, *machine_opts;
QemuOptsList *olist;
int optind;
const char *optarg;
@@ -2418,8 +2419,7 @@ int main(int argc, char **argv, char **envp)
cpu_model = NULL;
ram_size = 0;
snapshot = 0;
-cyls = heads = secs = 0;
-translation = BIOS_ATA_TRANSLATION_AUTO;
+snprintf(hdachs_params, sizeof(hdachs_params), "%s", HD_OPTS);
for (i = 0; i < MAX_NODES; i++) {
node_mem[i] = 0;
@@ -2467,7 +2467,7 @@ int main(int argc, char **argv, char **envp)
if (optind >= argc)
break;
if (argv[optind][0] != '-') {
- hda_opts = drive_add(IF_DEFAULT, 0, argv[optind++], HD_OPTS);
+hda_opts = drive_add(IF_DEFAULT, 0, argv[optind++], hdachs_params);
} else {
const QEMUOption *popt;
@@ -2485,21 +2485,8 @@ int main(int argc, char **argv, char **envp)
cpu_model = optarg;
break;
case QEMU_OPTION_hda:
-{
-char buf[256];
-if (cyls == 0)
-snprintf(buf, sizeof(buf), "%s", HD_OPTS);
-else
-snprintf(buf, sizeof(buf),
- "%s,cyls=%d,heads=%d,secs=%d%s",
- HD_OPTS , cyls, heads, secs,
- translation == BIOS_ATA_TRANSLATION_LBA ?
- ",trans=lba" :
- translation == BIOS_ATA_TRANSLATION_NONE ?
- ",trans=none" : "");
-drive_add(IF_DEFAULT, 0, optarg, buf);
-break;
-}
+hda_opts = drive_add(IF_DEFAULT, 0, optarg, hdachs_params);
+break;
case QEMU_OPTION_hdb:
case QEMU_OPTION_hdc:
case QEMU_OPTION_hdd:
@@ -2533,7 +2520,10 @@ int main(int argc, char **argv, char **envp)
break;
case QEMU_OPTION_hdachs:
{
+int cyls, heads, secs, translation;
const char *p;
+cyls = heads = secs = 0;
+translation = BIOS_ATA_TRANSLATION_AUTO;
p = optarg;
cyls = strtol(p, (char **)&p, 0);
if (cyls < 1 || cyls > 16383)
@@ -2565,7 +2555,14 @@ int main(int argc, char **argv, char **envp)
fprintf(stderr, "qemu: invalid physical CHS format\n");
exit(1);
}
- if (hda_opts != NULL) {
+snprintf(hdachs_params, sizeof(hdachs_params),
+ "%s,cyls=%d,heads=%d,secs=%d%s",
+ HD_OPTS , cyls, heads, secs,
+ translation == BIOS_ATA_TRANSLATION_LBA ?
+ ",trans=lba" :
+ translation == BIOS_ATA_TRANSLATION_NONE ?
+ ",trans=none" : "");
+if (hda_opts != NULL) {
char num[16];
snprintf(num, sizeof(num), "%d", cyls);
qemu_opt_set(hda_opts, "cyls", num);
--
1.7.10.2.484.gcd07cc5
[Qemu-devel] [PATCH v6 4/8] vga: add some optional CGA compatibility hacks
This patch adds some optional compatibility hacks (default
disabled) to allow Microport UNIX to function under qemu.
I've tried to structure it to be easy to add more hacks for other
old CGA programs, if anyone ever needs them.
Microport UNIX System V/386 v 2.1 (ca 1987) tries to program
the CGA registers directly with neither the assistance of BIOS, nor
with proper handling of EGA/VGA-only registers. Note that it didn't
work on real VGA hardware, either (although in that case, the most
obvious problems seemed to be out-of-range hsync and/or vsync
signalling, rather than the issues in this patch).
Eventually real MDA and/or CGA support might provide an alternative to
this patch, although a hybrid approach like this patch might still
be useful in marginal cases.
Signed-off-by: Matthew Ogilvie
---
hw/pc.h | 4
hw/vga.c| 37 +
qemu-options.hx | 19 +++
vl.c| 23 +++
4 files changed, 75 insertions(+), 8 deletions(-)
diff --git a/hw/pc.h b/hw/pc.h
index e4db071..37e2f87 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -176,6 +176,10 @@ enum vga_retrace_method {
extern enum vga_retrace_method vga_retrace_method;
+#define VGA_CGA_HACK_PALETTE_BLANKING (1<<0)
+#define VGA_CGA_HACK_FONT_HEIGHT (1<<1)
+extern int vga_cga_hacks;
+
static inline DeviceState *isa_vga_init(ISABus *bus)
{
ISADevice *dev;
diff --git a/hw/vga.c b/hw/vga.c
index afaef0d..15b5f0d 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -552,14 +552,31 @@ void vga_ioport_write(void *opaque, uint32_t addr,
uint32_t val)
printf("vga: write CR%x = 0x%02x\n", s->cr_index, val);
#endif
/* handle CR0-7 protection */
-if ((s->cr[VGA_CRTC_V_SYNC_END] & VGA_CR11_LOCK_CR0_CR7) &&
-s->cr_index <= VGA_CRTC_OVERFLOW) {
-/* can always write bit 4 of CR7 */
-if (s->cr_index == VGA_CRTC_OVERFLOW) {
-s->cr[VGA_CRTC_OVERFLOW] = (s->cr[VGA_CRTC_OVERFLOW] & ~0x10) |
-(val & 0x10);
+if (s->cr[VGA_CRTC_V_SYNC_END] & VGA_CR11_LOCK_CR0_CR7) {
+if (s->cr_index <= VGA_CRTC_OVERFLOW) {
+/* can always write bit 4 of CR7 */
+if (s->cr_index == VGA_CRTC_OVERFLOW) {
+s->cr[VGA_CRTC_OVERFLOW] =
+(s->cr[VGA_CRTC_OVERFLOW] & ~0x10) | (val & 0x10);
+}
+return;
+} else if ((vga_cga_hacks & VGA_CGA_HACK_FONT_HEIGHT) &&
+ !(s->sr[VGA_SEQ_CLOCK_MODE] & VGA_SR01_CHAR_CLK_8DOTS))
{
+/* extra CGA compatibility hacks (not in standard VGA) */
+if (s->cr_index == VGA_CRTC_MAX_SCAN &&
+val == 7 &&
+(s->cr[VGA_CRTC_MAX_SCAN] & 0xf) == 0xf) {
+return;
+} else if (s->cr_index == VGA_CRTC_CURSOR_START &&
+ val == 6 &&
+ (s->cr[VGA_CRTC_MAX_SCAN] & 0xf) == 0xf) {
+val = 0xd;
+} else if (s->cr_index == VGA_CRTC_CURSOR_END &&
+ val == 7 &&
+ (s->cr[VGA_CRTC_MAX_SCAN] & 0xf) == 0xf) {
+val = 0xe;
+}
}
-return;
}
s->cr[s->cr_index] = val;
@@ -1891,7 +1908,11 @@ static void vga_update_display(void *opaque)
/* nothing to do */
} else {
full_update = 0;
-if (!(s->ar_index & 0x20)) {
+if (!(s->ar_index & 0x20) &&
+/* extra CGA compatibility hacks (not in standard VGA) */
+(!(vga_cga_hacks & VGA_CGA_HACK_PALETTE_BLANKING) ||
+ s->ar_index != 0 ||
+ !s->ar_flip_flop)) {
graphic_mode = GMODE_BLANK;
} else {
graphic_mode = s->gr[VGA_GFX_MISC] & VGA_GR06_GRAPHICS_MODE;
diff --git a/qemu-options.hx b/qemu-options.hx
index d473e1c..5ba9c19 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1005,6 +1005,25 @@ Valid optional properties are
@item retrace=dumb|precise
Select dumb (default) or precise VGA retrace logic, useful for some
DOS games/demos.
+@item cga_hacks=@var{hack1}[+@var{hack2}[+...]]
+Enable various extra CGA compatibility hacks for programs that are
+trying to directly set CGA modes without BIOS assistance nor
+real knowledge of EGA/VGA. These might only work with -vga std.
+Valid hacks are
+@table @option
+@item palette_blanking
+Wait to blank the screen until palette registers seem to actually be
+modified, instead of blanking it as soon as the palette address bit (0x10)
+of the attribute address register (0x3c0) is cleared.
+@item font_height
+Ignore attempts to change the VGA font height (index 9),
+cursor start (index 10), and cursor end (index 11) of the CRTC control
+registers (0x3d5) if trying to set them to the default for CGA fonts
+instead of VGA fonts.
+@item all
+Enable
[Qemu-devel] [PATCH v6 1/8] fix some debug printf format strings
These are normally ifdefed out and don't matter. But if you enable
them, they ought to be correct.
Signed-off-by: Matthew Ogilvie
---
hw/cirrus_vga.c | 4 ++--
hw/i8259.c | 3 ++-
hw/ide/cmd646.c | 5 +++--
hw/ide/via.c| 5 +++--
4 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 9a0a565..107254d 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -2055,8 +2055,8 @@ static void cirrus_vga_mem_write(void *opaque,
}
} else {
#ifdef DEBUG_CIRRUS
-printf("cirrus: mem_writeb " TARGET_FMT_plx " value %02x\n", addr,
- mem_value);
+printf("cirrus: mem_writeb " TARGET_FMT_plx " value %02" PRIx64 "\n",
+ addr, mem_value);
#endif
}
}
diff --git a/hw/i8259.c b/hw/i8259.c
index 53daf78..6587666 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -355,7 +355,8 @@ static uint64_t pic_ioport_read(void *opaque,
target_phys_addr_t addr,
ret = s->imr;
}
}
-DPRINTF("read: addr=0x%02x val=0x%02x\n", addr, ret);
+DPRINTF("read: addr=0x%02" TARGET_PRIxPHYS " val=0x%02x\n",
+addr, ret);
return ret;
}
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index e0b9443..dd2855e 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -154,7 +154,7 @@ static uint64_t bmdma_read(void *opaque, target_phys_addr_t
addr,
break;
}
#ifdef DEBUG_IDE
-printf("bmdma: readb 0x%02x : 0x%02x\n", addr, val);
+printf("bmdma: readb 0x%02" TARGET_PRIxPHYS " : 0x%02x\n", addr, val);
#endif
return val;
}
@@ -170,7 +170,8 @@ static void bmdma_write(void *opaque, target_phys_addr_t
addr,
}
#ifdef DEBUG_IDE
-printf("bmdma: writeb 0x%02x : 0x%02x\n", addr, val);
+printf("bmdma: writeb 0x%02" TARGET_PRIxPHYS " : 0x%02" PRIx64 "\n",
+ addr, val);
#endif
switch(addr & 3) {
case 0:
diff --git a/hw/ide/via.c b/hw/ide/via.c
index b20e4f0..948a469 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -55,7 +55,7 @@ static uint64_t bmdma_read(void *opaque, target_phys_addr_t
addr,
break;
}
#ifdef DEBUG_IDE
-printf("bmdma: readb 0x%02x : 0x%02x\n", addr, val);
+printf("bmdma: readb 0x%02" TARGET_PRIxPHYS " : 0x%02x\n", addr, val);
#endif
return val;
}
@@ -70,7 +70,8 @@ static void bmdma_write(void *opaque, target_phys_addr_t addr,
}
#ifdef DEBUG_IDE
-printf("bmdma: writeb 0x%02x : 0x%02x\n", addr, val);
+printf("bmdma: writeb 0x%02" TARGET_PRIxPHYS " : 0x%02" PRIx64 "\n",
+ addr, val);
#endif
switch (addr & 3) {
case 0:
--
1.7.10.2.484.gcd07cc5
[Qemu-devel] [PATCH v6 6/8] i8259: fix so that dropping IRQ level always clears the interrupt request
Intel's definition of "edge triggered" means: "asserted with a
low-to-high transition at the time an interrupt is registered and
then kept high until the interrupt is served via one of the
EOI mechanisms or goes away unhandled."
So the only difference between edge triggered and level triggered
is in the leading edge, with no difference in the trailing edge.
This bug manifested itself when the guest was Microport UNIX
System V/386 v2.1 (ca. 1987), because it would sometimes mask
off IRQ14 in the slave IMR after it had already been asserted.
The master would still try to deliver an interrupt even though
IRQ2 had dropped again, resulting in a spurious interupt
(IRQ15) and a panicked kernel.
Output from a test program:
---
Real hardware [Pentium 4]:
cmdRead unmask IRR=4005 mask IRR=4001 sti unmask irq14 IRR=0001 DONE
[I also see a final IRR= occasionally. Probably just happened to
ask it while the timer (IRQ0) line is low. It masks off most IRQ's, including
timer.]
---
Unpatched qemu:
cmdRead unmask IRR=4015 mask IRR=4015 sti irq15 unmask IRR=4015 DONE
[Presumably IRQ4 (0x10 - qemu's serial device model?) had a transient
edge during initialization, but had been masked off even before I
masked it off?]
---
Patched qemu:
cmdRead unmask IRR=4005 mask IRR=4001 sti unmask irq14 IRR=0001 DONE
---
Signed-off-by: Matthew Ogilvie
---
hw/i8259.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/i8259.c b/hw/i8259.c
index 6587666..c011787 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -157,6 +157,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
}
s->last_irr |= mask;
} else {
+s->irr &= ~mask;
s->last_irr &= ~mask;
}
}
--
1.7.10.2.484.gcd07cc5
[Qemu-devel] [PATCH v6 5/8] i8254: fix inaccuracies in pit_get_out()
* Fix high-vs-low counting logic to match the timing diagrams
and descriptions in Intel's documentation (23124406.pdf).
* Improve reading back the count in mode 3.
* Handle GATE input properly with respect to the OUT line, and add
a FIXME comment for reading back the counter. GATE is hard wired
high for channel 0 (IRQ0), but it can be software controlled on
channel 2 (PC speaker).
The output line is now high much more often than before, especially
in modes 2 and 4, which might cause updates of the timer chip
to generate extra interrupts. But this should only be an issue for
some migration cases.
Signed-off-by: Matthew Ogilvie
---
There are still some things not quite modeled correctly.
See the cover letter of this patch series for details, and
the FIXME comments added to the code.
hw/i8254.c| 24 ++--
hw/i8254_common.c | 18 ++
2 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/hw/i8254.c b/hw/i8254.c
index 77bd5e8..9168016 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -39,6 +39,15 @@ static void pit_irq_timer_update(PITChannelState *s, int64_t
current_time);
static int pit_get_count(PITChannelState *s)
{
+/* FIXME: Add some way to represent a paused timer and return
+ * the paused-at counter value, to better model:
+ * - gate-triggered modes (1 and 5),
+ * - gate-pausable modes,
+ * - [maybe] the "wait until next CLK pulse to load counter" logic,
+ * - [maybe/not clear] half-loaded counter logic for mode 0, and
+ * the "null count" status bit,
+ * - etc.
+ */
uint64_t d;
int counter;
@@ -52,8 +61,7 @@ static int pit_get_count(PITChannelState *s)
counter = (s->count - d) & 0x;
break;
case 3:
-/* XXX: may be incorrect for odd counts */
-counter = s->count - ((2 * d) % s->count);
+counter = (s->count - ((2 * d) % s->count)) & 0xfffe;
break;
default:
counter = s->count - (d % s->count);
@@ -98,6 +106,18 @@ static inline void pit_load_count(PITChannelState *s, int
val)
if (val == 0)
val = 0x1;
s->count_load_time = qemu_get_clock_ns(vm_clock);
+
+/* In gate-triggered one-shot modes, indirectly model some pit_get_out()
+ * cases by setting the load time way back until gate-triggered.
+ * (Generally only affects reading status from channel 2 speaker,
+ * due to hard-wired gates on other channels.)
+ *
+ * FIXME: This might be redesigned if a paused timer state is added
+ * for pic_get_count().
+ */
+if (s->mode == 1 || s->mode == 5)
+s->count_load_time -= muldiv64(val+2, get_ticks_per_sec(), PIT_FREQ);
+
s->count = val;
pit_irq_timer_update(s, s->count_load_time);
}
diff --git a/hw/i8254_common.c b/hw/i8254_common.c
index a03d7cd..dc13c99 100644
--- a/hw/i8254_common.c
+++ b/hw/i8254_common.c
@@ -50,24 +50,18 @@ int pit_get_out(PITChannelState *s, int64_t current_time)
switch (s->mode) {
default:
case 0:
-out = (d >= s->count);
-break;
case 1:
-out = (d < s->count);
+out = (d >= s->count);
break;
case 2:
-if ((d % s->count) == 0 && d != 0) {
-out = 1;
-} else {
-out = 0;
-}
+out = (d % s->count) != (s->count - 1) || s->gate == 0;
break;
case 3:
-out = (d % s->count) < ((s->count + 1) >> 1);
+out = (d % s->count) < ((s->count + 1) >> 1) || s->gate == 0;
break;
case 4:
case 5:
-out = (d == s->count);
+out = (d != s->count);
break;
}
return out;
@@ -93,10 +87,10 @@ int64_t pit_get_next_transition_time(PITChannelState *s,
int64_t current_time)
break;
case 2:
base = (d / s->count) * s->count;
-if ((d - base) == 0 && d != 0) {
+if ((d - base) == s->count-1) {
next_time = base + s->count;
} else {
-next_time = base + s->count + 1;
+next_time = base + s->count - 1;
}
break;
case 3:
--
1.7.10.2.484.gcd07cc5
[Qemu-devel] [PATCH v6 7/8] i8259: refactor pic_set_irq level logic
No change in functionality.
Clarify that the only difference between level triggered and
edge triggered interrupts is on the leading edge.
Signed-off-by: Matthew Ogilvie
---
hw/i8259.c | 26 +-
1 file changed, 9 insertions(+), 17 deletions(-)
diff --git a/hw/i8259.c b/hw/i8259.c
index c011787..1ba9b3a 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -140,26 +140,18 @@ static void pic_set_irq(void *opaque, int irq, int level)
}
#endif
-if (s->elcr & mask) {
-/* level triggered */
-if (level) {
+if (level) {
+if ((s->last_irr & mask) == 0 || /* edge for edge triggered */
+(s->elcr & mask)) { /* or level triggered */
s->irr |= mask;
-s->last_irr |= mask;
-} else {
-s->irr &= ~mask;
-s->last_irr &= ~mask;
}
+s->last_irr |= mask;
} else {
-/* edge triggered */
-if (level) {
-if ((s->last_irr & mask) == 0) {
-s->irr |= mask;
-}
-s->last_irr |= mask;
-} else {
-s->irr &= ~mask;
-s->last_irr &= ~mask;
-}
+/* Dropping level clears the interrupt regardless of edge trigger
+ * vs level trigger.
+ */
+s->irr &= ~mask;
+s->last_irr &= ~mask;
}
pic_update_irq(s);
}
--
1.7.10.2.484.gcd07cc5
[Qemu-devel] [PATCH v6 0/8] i8254, i8259 and running Microport UNIX (ca 1987)
Changes since previous version: * Patches 1, 2, 3, and 4, and 7 haven't changed at all. * The only change to patch 6 is I added test cases output to the commit message. * Patches 5 and 8 are new. Patch 8 has downsides; see migration notes below. Disclaimer: The PIT patches and the analysis below are mostly based on reading the spec and the code, with only a minimal amount of sanity testing so far. - Remaining inaccuracies in the qemu (user mode) 8254 PIT (timer) model after these patches: * Generally, a new guest-supplied initial count is only loaded into the actual counter on the next CLK tick. This could be delayed by about a microsecond (CLK runs at about 1.1 MHz). * CLK ticks should be predefined; nothing done in software should change their timing. But we essentially restart CLK (throwing out fractional ticks) whenever the guest programs the PIT. * It doesn't model a paused counter (for example, when GATE is low), and reading back the paused count from the counter. Also, when it is unpaused, it always starts back at the beginning, rather than where it was paused. This shouldn't affect interrupts (channel 0's GATE is hard-wired high), but it could affect reading back the count from channel 2 (PC speaker). * Lost tick risks: * When the guest leaves interrupts disabled for an extended period of time, the level might return low before the guest sees the inturrpt. This can also happen on real hardware, so probably isn't worth worrying about. * Depending on the underlying qemu-timer model (used for CLK), there might be a risk that the timer could skip a significant chunk of time without providing any processor time to the 8254 model or the guest (perhaps if the host is scheduling other processes). I'm not sure if this is possible or likely, but if it happens, the 8254 will dutifully provide all the missing edges to the 8259. But the 8254 will provide them all at once (until it catches up to the qemu-timer time), and the 8259 and guest will effectively not notice nor process the earlier edges in the batch. Again, not sure when/if this can actually happen. Maybe it is more likely to happen with KVM in the "kernel_irqchip=off" case, but not in the native case? Not sure. My general impression is that none of these are worth worrying about. They are fairly tangential to any corrections to the interrupt trailing edge model. - Question about best way to handle migration issues: There is a tradeoff between potentially losing an IRQ0 tick in some cases, vs potentially gaining an IRQ0 tick in other cases, and how to deal with interrupts. NOTE: In addition to the lost leading edge issue mentioned in the comment in the patch, some of my changes in patch 5 fixes where the leading edge occurs by 1 CLK count. Migration might risk delivering an interrupt twice if it happens in the 1 CLK interval. Evaluate ways of dealing with this. Perhaps intentionally leave the off-by-one in perpetuity, or is there a clever way to avoid issues? I'm not sure how to resolve it, but here are some ideas: 1. Leave out patch 8 and do nothing. Potentially lose an interrupt when migrating back to an older version, because the old version always expects to be able to produce a leading edge by simply setting IRQ0 high, but if it is normally high... 2. Use patch 8. If we keep it, it probably belongs near patch 5 or 6, or squashed in with one of them. But if the guest uses something like the one-shot mode 4, it would get an extra interrupt whenever it restarts the timer if the previous interrupt has been serviced. (Some of these could match real hardware, if mode 4 happens to be started when IRQ0 happens to be low. But this is relatively unlikely.) 3. Use something similar to patch 8, but only in the exported data used for migration, not in normal operation. This could possibly generate a single extra interrupt when migrating, but with luck most guests should be able to handle that. 4. Or only prepare to fix it sometime in the future: * Explicitly do not fix the trailing edge logic for IRQ0 in the 8259. Only fix other IRQ's for now. * Leave the PIT functionally mostly as-is, except: * Try to tweak the PIT model so that it could handle migration to/from some future fixed version without problems. For example, when it wants to generate a leading edge, make sure it is a leading edge by first explicitly lowering the IRQ, even if it "should have" been low already. * Wait until sometime in the future (years from now?) when back-migrating to 2012 version is no longer important to actually fix the PIT interrupt level model. 5. Or implement two models of operation, and a co
[Qemu-devel] [PATCH v6 3/8] qemu-options.hx: mention retrace= VGA option
The feature was added in commit cb5a7aa8c32141bb Sep 2008.
My description is based on "Better VGA retrace emulation (needed
for some DOS games/demos)" from
http://www.boblycat.org/~malc/code/patches/qemu/index.html
Signed-off-by: Matthew Ogilvie
---
qemu-options.hx | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/qemu-options.hx b/qemu-options.hx
index 7d97f96..d473e1c 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -975,7 +975,7 @@ DEF("vga", HAS_ARG, QEMU_OPTION_vga,
"-vga [std|cirrus|vmware|qxl|xenfb|none]\n"
"select video card type\n", QEMU_ARCH_ALL)
STEXI
-@item -vga @var{type}
+@item -vga @var{type}[,@var{prop}=@var{value}[,...]]
@findex -vga
Select type of VGA card to emulate. Valid values for @var{type} are
@table @option
@@ -1000,6 +1000,12 @@ Recommended choice when using the spice protocol.
@item none
Disable VGA card.
@end table
+Valid optional properties are
+@table @option
+@item retrace=dumb|precise
+Select dumb (default) or precise VGA retrace logic, useful for some
+DOS games/demos.
+@end table
ETEXI
DEF("full-screen", 0, QEMU_OPTION_full_screen,
--
1.7.10.2.484.gcd07cc5
[Qemu-devel] [PATCH v6 8/8] i8259/i8254: migration workaround for timer
Signed-off-by: Matthew Ogilvie
---
It is not at all clear that this is the best way to handle this.
See the detailed notes in the cover letter of this patch series.
UPDATE: Also, some fixes moved the leading edge by 1 CLK
tick (CLK ticks at about 1.1 MHz), and some strategies like this
might risk extra interrupts just from that.
--
hw/i8259.c | 39 +++
1 file changed, 39 insertions(+)
diff --git a/hw/i8259.c b/hw/i8259.c
index 1ba9b3a..4b3c6c9 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -153,6 +153,45 @@ static void pic_set_irq(void *opaque, int irq, int level)
s->irr &= ~mask;
s->last_irr &= ~mask;
}
+
+/* Back-migration compatibility hack:
+ * As of late 2012, the PIT timer model was incorrectly
+ * pulsing the the IRQ0 line high for only a short time to
+ * indicate an interrupt. It counted on a conceptual bug in
+ * the PIC irq model to latch onto and and deliver the
+ * interrupt even after it became low again. (Normally lowering
+ * an IRQ line before it is serviced should cancel the
+ * interrupt.)
+ *
+ * In late 2012 the model has been improved to match hardware
+ * much better by only pulsing low for a short time (in most
+ * PIT modes), but unfortunately that means if you back-migrate
+ * a guest to a version without this fix, the next interrupt
+ * won't have its own leading edge at all, and will
+ * be lost.
+ *
+ * The following hack will allow both the current
+ * interrupt to be serviced properly, and the next one
+ * as well, regardless of which version the migration is
+ * restored on.
+ *
+ * Unfortunately, this has a small possibility of causing
+ * an extra IRQ0 in cases that would not have in the old 2012
+ * model, nor on real hardware. Specifically, if the current
+ * interrupt is processed, and then something causes an
+ * pit_irq_timer_update() to the same high level it was previously
+ * updated with. Re-setting various PIT modes (like 4) could
+ * do this, for example.
+ *
+ * At some point in the future (years from now?),
+ * when back-migration to the old 2012 version is
+ * no longer important, it should be safe to just delete
+ * this hack.
+ */
+if (irq==0 && s->master) {
+s->last_irr &= ~1;
+}
+
pic_update_irq(s);
}
--
1.7.10.2.484.gcd07cc5
Re: [Qemu-devel] [PATCH 09/17] aio: prepare for introducing GSource-based dispatch
Il 29/09/2012 13:28, Blue Swirl ha scritto: >> > +if (node->pfd.fd == fd) > Forgot to run checkpatch.pl? > No, just ignored its result for an RFC. Paolo
